Форум PHP

Написал класс загрузки файлов на сервер. Пока что предусмотрен вариант проверки файла по расширению, чуть позже сделаю по mime-типу. Я только разбираюсь с ООП, поэтому хотелось бы обострить ваше внимание именно насколько целесообразно и насколько правильно или неправильно я использовал возможности ооп.
Я готов выслушать конструктивную критику, поскольку на основе ваших анализов, я могу сделать некоторые выводы, которые помогут мне более существенно усовершенствовать класс.
Ниже прилагаю класс и вариант его использования.

<? class UploadedFile {          private $path;      private $file;      private $name;      private $type;      private $tmp_name;      private $error;      private $size;      private $arrExt;      private $uploadedFile;      // метод — конструктор      function __construct($file) {          $this->name     = $file['name'];          $this->type     = $file['type'];          $this->tmp_name = $file['tmp_name'];          $this->error     = $file['error'];          $this->size     = $file['size'];      }      // метод, возвращающий имя файла      public function getName() {          return $this->name;      }      //метод, возвращающий тип файла      public function getType() {          return $this->type;      }      // метод, возвращающий адрес временного файла      public function getTmpFile() {          return $this->tmp_name;      }      //метод, возвращающий расширение файла      public function getExt() {          return end(explode('.',$this->name));      }      // метод возвращающий номер ошибки, если она есть      public function getErrors() {          return $this->error;      }      // метод, возвращающий размер файла      public function getSize() {          return $this->size;      }      // метод позволяет загрузить файл на сервер      public function move_uploaded($uploadedFile) {          $this->$uploadedFile = ltrim($uploadedFile,'/');            if(!is_dir(dirname($this->$uploadedFile))) {                $dir = dirname($this->$uploadedFile);                    if(!mkdir($dir,0700,true)) {                        return false;                    }                                    }          // перемещаем файл          if(move_uploaded_file($this->tmp_name,$this->$uploadedFile)) {               return true;          }           else {              return false;          }      }    /*      Данный метод позволяет разрешить, либо запретить загрузку файла на сервер в зависимости от расширения файла.      В качестве первого аргумента передается массив $arrExt, в котором перечисляются множество расширений.      В качестве второго параметра $action  передается действие:      1) если передано true, то загрузка файла с расширениями указанные в массиве $arrExt разрешена, иначе нет;      2) если передано false, соответсвенно загрузка с такими расширениями запрещена    */      public function check_exp($arrExt = null,$action = true) {         if(is_array($arrExt)) {             $this->$arrExt = $arrExt;             foreach($this->$arrExt AS $exts) {                    $arrext[] = strtolower(ltrim($exts,'.'));             }             $this->$arrExt = $arrext;             if(in_array(self::getExt(),$this->$arrExt)) {                 // такое расширение присутсвует                 if($action === true) {                     return true;                 }                 else {                     return false;                 }             }             else {                 return false;             }         }         else {             return false;         }      }    }  if(!empty($_FILES['file']['name'])) {       // пример использования класса       // например, нужно загрузить файлы формата doc и xls, остальные не допускаются       $upl = new UploadedFile($_FILES['file']);       if($upl->check_exp(array('doc','xls'),true)) {           // дирректория, в которую загружается файл, если ее нет, то класс автоматически создас, при этом глубина вложенности может быть любая           $dir = $_SERVER['DOCUMENT_ROOT'].'files/docs/';           $namefile = time().'.'.$upl->getExt;           // загружаем файл           if($upl->move_uploaded($dir.$namefile)) {               echo 'Файл успешно загружен!';           }           else {               echo 'Ошибка загрузки файла';           }           }           else {             echo 'Файлы с имеет недопустимое расширение — '.$upl->getExt();         }     } ?>

Хорошо бы вот эту строку

<?php  if(!empty($_FILES['file']['name'])) { ?>

реализовать как-то средствами класса, т.е. реализовать инкапсуляцию, спрятать детали внутрь класса, предоставляя пользователю-программисту только интерфейс класса. А так получается, что пользователю нужно понимать детали процесса загрузки файла, зачем тогда класс? Да и move_uploaded() лучше уничтожить и переместить $dir.$namefile в параметр конструктора. Ясно дело, что никому файл во временной директории не нужен - беретесь выполнять работу загрузки файла - выполняйте её до конца и не заставляйте пользователей вникать в детали этого процесса. Пусть будет конструктор и возможность проверки удачно файл загружен или нет, остальное пожалуй и лишнее.

PS Хорошо бы HTML-форму привести, чтобы посмотреть как это в реальности работать будет.

Первое, что сразу бьет по глазам - огромный, насыщенный аргумент конструктора.
Конструктор имеет смысл сделать так, чтобы процесс, который пользуется этим классом, передавал в него минимум информации. Например, значение атрибута имени поля формы.

Предполагается, что конструктору скармливается элемент класса $_FILES, т.е. уже заполенный. Думаю тут подход оправдан, пользователю не нужно будет заботиться о подготовке сложного параметра.

Вот именно, что элемент, а не его ключ.
Зачем тащить громоздкий элемент, когда достаточно ключа?

А меж тем, при множественных файловых полях, в самом суперглобальном массиве вовсе не всё так очевидно.
Уж коль делать класс, так пусть бы и разбирался с этой кашей, как Вы, собственно, и предложили.

А если файлов несколько и они сами в массиве? Тогда класс не сможет корректно извлечь данные из массива $_FILES и будет не удобно, придется их обрабатывать по старинке...

>А если файлов несколько и они сами в массиве?

Вот именно.


>Тогда класс не сможет корректно извлечь данные из массива $_FILES и будет не удобно, придется их обрабатывать по старинке...

Что значит не может?
Браузер смог запихать, а класс не может?
Пусть выдирает пока не вынет.

PS. Пардон.. я за стол!
С наступающим! :)))

>PS. Пардон.. я за стол!
>С наступающим! :)))
Да я тоже :))) И вас с наступающим!

Хм, Вы правы, я это как-то упустил. Переделаю)

>Данный метод позволяет разрешить, либо запретить загрузку файла на сервер в зависимости от расширения файла.

Расширение, это блеф. ;-)

Почему Вы так решили? Я сейчас несколько поразмыслил, и пришел к выводу что действительно здесь насовсем правильно реализован конструктор класса, поскольку реализация метода множественной загрузки будет трудновата.
Еще я хочу сделать проверку по mime-типу.

Да потому, что я могу любой файл обозвать любым расширением, и вы его "скушаете" своим классом :)

Ну если вы внимательно изучили класс, то вы поймете наконец что в классе можно обозначить список допустимых расширений))))

Вы не поняли.
Разрешенный может быть только .jpg, а я решу залить php скрипт, и просто поменяю его расширение на .jpg и этот класс будет считать мой скрипт картинкой и пропустит его.

А что там изучать? Расширение, это блеф.

Содержимое, если по большому счету, тоже блеф.
На что ориентируется http-сервер, выставляя Content-Type?

На расширение.

1. имхо, все ваши privat-члены не нужны, нужен только один член-массив, скажем, $_files.
я бы передавал конструктору массив $_FILES, далее привел бы его в двумерный массив (если надо) и присвоил бы все это дело аттрибуту $_files.

2. ваша валидация... а что если надо будет проверить не только расширение, но и объем?
добавьте список валидаторов в ваш класс. для этого добавьте аттрибут $_validators = array(); + методы addValidtor($validator), isValid()

>...я бы передавал конструктору массив $_FILES,...
Массив $_FILES - суперглобальный.

а чем плохо передавать его конструктору?
если вы о приведении в двумерный - так привести надо не суперглобальный $_FILES, а член класса $_files

Не в конструкторе дело...

Нет смысла.

Нет никакого смысла передавать куда угодно объект, который однозначно определен, и доступен на любом уровне, в любой области видимости.

да, согласен, что смысла нет, поторопился с ответом
просто, в таком случае мне хочется назвать этот класс UploaderHttp и сделать его адаптером

upd. кстати, в зф примерно такая реализация

Я несколько переработал класс, но пока он еще не готов на публичное обозрение. Благодаря вашему внимаю проявленному в теме, я несколько изменил класс, в частности прислушался к словам Trianon'а и в конструктор класс передавал только атрибут поля формы.
Сейчас я работаю на методом множественной загрузки файла. Собственно по-моему мнения я не хочу добавлять в класс метод для множественной загрузки файла, а усовершенствовать существующий.
Что касается безопасности загрузки файла, то я решил добавить валидаторы, как отдельные методы (то есть будет предусмотрено несколько вариантов проверки файла), при этом не всегда требуется проверять файл (бывает случаи когда класс нужно использовать для разовых задач и только для себя).
Вообщем хочу показать примерный вариант реализации множественной загрузки файла и опять же хотелось бы услышать ваши рекомендации насколько правильно и удобно его использовать

<?          // пример множественной загрузки файла          $upl = new UploadedFile('File');         // возвращает многомерный массив, содержащий такие же ключи как и у $_FILES(name,size и.т.д)          $arr = $upl->getFile();          // кол-во загружаемых файлов          $count = $upl->getCountFile();          // поочередно копируем файл на сервер          for ($i=0;$i<$count;$i++) {              $newfilename = time().'.'.$upl->getExt($i);              if($upl->move_uploaded($newfilename)) {                  echo 'Файл успешно загружен - '.$newfilename.' <br />';              }              else {                  echo 'Ошибка загрузки файла<br />';              }          } ?>

Может, возможно реализацию сделать еще проще?

if($upl->move_uploaded($newfilename)) {    echo 'Файл успешно загружен - '.$newfilename.' <br />';  } else {    echo 'Ошибка загрузки файла<br />';  }

При загрузке нескольких файлов откуда знать пользователю какой файл на данный момент загружался и не смог загрузиться? При чем по какой причине? Если вы виновник этого, как то забыли о правах на директорию, то при чем тут пользователь, зачем ему ваша ошибка?

Sim5, для этого в классе описан метод getName() (он несколько переработан мной)
Я не стал подробно расписывать возможности, меня просто интересует насколько правильно использовать варинт использования метода в цикле (for)

А узнать имя текущего файла и прочую инфу, доступную в массиве $_FILES можно используя методы класса —

<?           // пример множественной загрузки файла           $upl = new UploadedFile('File');          // возвращает многомерный массив, содержащий такие же ключи как и у $_FILES(name,size и.т.д)           $arr = $upl->getFile();           // кол-во загружаемых файлов           $count = $upl->getCountFile();           // поочередно копируем файл на сервер           for ($i=0;$i<$count;$i++) {               $newfilename = time().'.'.$upl->getExt($i);               if($upl->move_uploaded($newfilename)) {                   echo 'Файл успешно загружен - '.$newfilename.' <br />';               }               else {                   echo 'Ошибка загрузки файла<br />';                   echo $upl->getName($i).'</br>'; // текущееимя файла                  $upl->getError($i).'</br>'; // номер ошибки                                  }                echo '<hr>';          }  ?>

Я не знаю чего вы пишите, я просто не вижу, и в данном случае, после "для этого в классе описан метод getName() ....", почему ошибка, кто виновен, не вижу проверок условий, и есть ли они вообще.... Загрузку (перемещение из временной директории) нужно производить после того, как убедимся что нет ошибок. И что ответить? Дело то не в цикле, можно и foreach, а как раз в последовательности обработки файла загружаемого, я лично у вас ее не вижу. А уж класс ли это, функция ли, это дело десятое. А ext, еще раз повторить, это халтура.

Вот о каких "высоких материях" можно рассуждать, если вы сперва пытаетесь переместить файл, а только потом узнаете били ли ошибки при загрузке? А если была, например файл загружен не полностью, либо вообще не загружен, то зачем и что перемещать? Ну где логика?

С foreach не очень красиво получится, придется внутри еще один foreach использовать.
А по поводу проверки, то в классе все предусмотренно, это я поспешил .

<? if(empty($upl->getError($i))) {   // нету никаких ошибок } else {    // есть ошибки echo $upl->getError($i); } ?>

Только я вам сейчас не про то говорю. Все эти проверки я сделаю на высшем уровне А про то как более правильно реализовать метод множественной загрузки файла используя ООП.

Это почему еще один foreach потребуется?
Должно быть if(!$upl->getError($i)), и тогда перемещение (зачем empty, отсутствие ошибки это 0, вы же ошибки массива проверяете), а у вас кверху каком, и так во всех диалогах, встает вопрос как же и что вы пишите.

А как правильно просто ее реализовать без ООП? Правильно, это тогда, когда логична последовательность действий, чего у вас не наблюдается, а только оговорки, что подождите, будет высший уровень... Этот уровень сразу должен быть заложен, он и определит правильность, а в ООП или просто, все равно циклом.

>Это почему еще один foreach потребуется?
Потому что массив многомерный. Или я ошибаюсь? Если да, то пожалуйста продемонстрируйте способ

Нет, я не о разложении чинном подразумевал, задавая вопрос. Да, for удобнее в плане разложения для этого массива, но ведь кроме циклов сущестуют и другие инструменты, вот они и помогут. Так что это как подойти, можно и вообще отказаться от цикла при обработке файлов, которые загружены, а имеющие ошибки вернуть массиву.

В описании метода
check_exp
увидел такую строчку:
if(in_array(self::getExt(),$this->$arrExt)) {
метод getExt() - метод объекта, а вы его пытаетесь использовать как метод класса (статический)... Но php ошибки не выдаст, я думаю) Но лучше поправиться...
И, кстате, я бы сделал этот метод как private

Этот я так, бегло пробежался :о)

И я так понял, что переменная
$newfilename = time().'.'.$upl->getExt($i);
Генерирует имя файла? Тогда можно создать отдельный метод для этого.

Да и вообще создать отдельный метод, который реализует ваш "move_upload" всех файлов..

Я правильно понимаю, что если сделать метод приватным, то он будет доступен только внутри самого класса?

...он будет доступен только объектам этого класса.
Более того, он не будет доступен даже детям при наследовании.
Поэтому уровень доступа лучше ставить protected - вдруг кто-то решит унаследовать ваш класс?

На самом деле внутри класса. Объектам этого класса нельзя будет обращаться напрямую к приватным метода...
Если понимать вас, то объекту ($temp) класса (Temp) доступен метод (someMethod)...
И всё таки, только внутри данного класса атрибут private доступен
А кстате, не все методы обязательно в protected описывать.. Хотя это может быть и целесообразно при наследовании в конкретных случаях использовать protected

>Я несколько переработал класс, но пока он еще не готов на публичное обозрение.

То есть, другими словами, говорить пока не о чем?

>Вообщем хочу показать примерный вариант реализации множественной загрузки файла и опять же хотелось бы услышать ваши рекомендации насколько правильно и удобно его использовать

Насколько правильно и удобно - сказать трудно.
Про множественную загрузку вообще трудно говорить, так как php к ней оказывается не приспособлен чуть менее чем полностью. Просто в силу того, что на совокупный объем файлов накладываются те же ограничения, что и на прочие POST-запросы. Но это, понятное дело, вопрос не к Вам.
К Вам вопросов два.

>

<?   > >       // пример множественной загрузки файла >         $upl = new UploadedFile('File'); >        // возвращает многомерный массив, содержащий такие же ключи как и у $_FILES(name,size и.т.д)

1. Если на выходе Вашего инструмента получается то же, что лежит в $_FILES, то какой в нем профит? Только формирование массива атрибутов по имени поля?
Как-то скудно это для целого класса...

><? >           $arr = $upl->getFile(); >         // кол-во загружаемых файлов >         $count = $upl->getCountFile(); >         // поочередно копируем файл на сервер >         for ($i=0;$i<$count;$i++) {

2. Вот эта строка:

><? >             $newfilename = time().'.'.$upl->getExt($i);

здесь явная ошибка.
файлы с одинаковыми расширениями будут получать одинаковые имена.

<? >             if($upl->move_uploaded($newfilename)) { >                 echo 'Файл успешно загружен - '.$newfilename.' <br />'; >             } >             else { >                 echo 'Ошибка загрузки файла<br />'; >             } >         } >?> >

вообще, этот фрагмент показывает, что рекомендацию cheops'а (31.12.2010 в 23:36) Вы пропустили мимо ушей.
Зачем мне самому вызывать move_uploaded, если есть класс?
Какая имено ошибка обнаружилась при загрузке?

>Может, возможно реализацию сделать еще проще?

лично с моей точки зрения, этот код перегружен скобками, и от того его тяжело воспринимать.
Но тут дело вкуса.

>Зачем мне самому вызывать move_uploaded, если есть класс?
Вы полагаете что, класс должен сам вызвать процесс перемещения файла на сервер? Такое же можно сделать только в момент создания объекта класса?

ну в принципе, в качестве альтернативного метода, можно оставить и такой.

В большинстве же случаев, наверняка, перемещение загруженного файла, выполняемое на лету, будет только удобно.
В крайнем случае, можно предусмотреть метод, формирующий целевое имя файла.

>>Зачем мне самому вызывать move_uploaded, если есть класс?
>Вы полагаете что, класс должен сам вызвать процесс перемещения файла на сервер? Такое же
>можно сделать только в момент создания объекта класса?
А что в этом плохого? Пусть конструктор проверяет наличие файла в $_FILES - если есть, пусть перемещает файл, если нет - пусть ничего не делает. В конце концов вряд ли кто-то будет создавать объект класса, если ему не требуется загрузка файла в конечную директорию. Чем меньше манипуляций для конечного пользователя - тем удобнее и эфективнее спроектирован интерфейс класса.

>>>Зачем мне самому вызывать move_uploaded, если есть класс?
>>Вы полагаете что, класс должен сам вызвать процесс перемещения файла на сервер? Такое же
>>можно сделать только в момент создания объекта класса?
>А что в этом плохого? Пусть конструктор проверяет наличие файла в $_FILES - если есть, пусть перемещает файл, если нет - пусть ничего не делает.

Тогда вариант с множественной загрузкой теряет всякий смысл. Например, я хочу при перемещении файла на сервер задавать ему новое имя для каждого файла, которое будет генерировать, например, какой-нибудь другой скрипт.

Создайте для генерации имен класс, и передайте объект этого класса в конструктор, если не хотите перегружать класс загрузки файлов дополнительной функциональностью. А еще лучше унаследуйте от класса загрузки файла/файлов, новый класс, где и реализуйте генерацию новых имен. Понадобится новый механизм генерации имен - унаследуйте еще один класс... Собственно ради подобных задач ООП-инструменты и создавались.

В таком случаи я бы советовал ввести, например, дополнительные параметры, которые бы указывали - делать все автоматически (вы передаете только индекс массива $_FILES, а на выходе сразу получаете имена загруженных файлов), либо нет, когда вы вручную задаете имя и т.д.
Правда во втором слуаи встает вопрос - а нафига класс то нужен?

Если говорить про один public-метод, то мне кажется, что в таком случае лучше и вовсе не создавать объект.
Реализовать тогда один интерфейсный статический метод с 1 параметром - $_FILES.
конструктор сделать приватным.
Для данного класса это можно сделать и таким образом

>Реализовать тогда один интерфейсный статический метод с 1 параметром - $_FILES.
Смысле передавать $_FILES? Почему? Почитайте выше сообщения Trianon'а

С $_FILES ошибся. Тогда можно сделать и вовсе без параметра.
cheops.. Некоторые кстате утверждают, что лучше вместо многократных наследований использовать композицию.. Хотя каждый сам для себя определяет соотношение использования одного и другого

>>Некоторые ...
тАк их наверное еще никто не называл:)
согласен про композицию, к тому же, если этот класс нагрузить функцией переименования - имхо нарушается принцип единственной ответственности
я бы создал массив фильтров в классе (по аналогии с валидаторами)

От задач зависит... где-то композиция выгодна, где-то наследование. Если имеется явная разветвленная структура - наследование будет очень кстати. Если классы достаточно замкнуты и самодостаточны - композиция будет более эффективна. К сожалению, менее расплывчатые критерии привести сложно, проектирование - это довольно творческий процесс, тут одной аналитикой не обойдешься, много параметров: для слабой команды будет выгодна одна стратегия, для сильной другая, если мало времени - лучше действовать одним путем, если времени достаточно - выгоднее идти другим. И таких параметров очень много...

-