Witam, mój pierwszy skrypt, klasa...

Dobrze załapałem póki co programowanie obiektowe? Jakie błędy robie? Co poprawić?

[PHP] pobierz, plaintext 
class UploadFiles{
    private $file;
    private $dir;
    private $mime = array('image/jpeg', 'image/gif', 'image/png');
    private $chmod=777;
 
    public function  saveFile($fileName, $dir) {
        $this->file=$_FILES[$fileName];
        $this->dir=$dir.'/'.$this->file['name'];
 
        if(isset($_FILES[$fileName])){
 
            if($this->file['error']==0){
                if(is_uploaded_file($this->file['tmp_name'])){
                    if(in_array($this->file['type'], $this->mime)){
                        if(move_uploaded_file($this->file['tmp_name'], $this->dir)===true){
                            chmod($this->dir, $this->chmod);
                            return true;
                        }
                    }else{
                        //Nieprawidłowy format pliku
                    }
                }
            }else{
                switch($_FILES[$this->file]['error']){
                    case 1: /* KOMUNIKAT o przekroczeniu wartości upload_max_filesize*/; break;
                    case 2: /* KOMUNIKAT o przekroczeniu wartości max_filesize*/; break;
                    case 3: /* KOMUNIKAT o wysłanym częściowo pliku*/; break;
                    case 4: /* KOMUNIKAT o nie wysłaniu żadnego pliku*/; break;
                }
            }
        }else{
            return false;
        }
    }
 
}
[PHP] pobierz, plaintext 

Źle...
1) Cały obiekt składa się wyłącznie z jednej metody, co za tym idzie nie ma sensu korzystać z pól/własności/składowych/jakkolwiek by tego nie nazwać klasy, skoro używane jest to jedynie w kontekście lokalnym
2) Takie sprawdzanie typu mime to o kant d..y rozbić można. _FILES[abc][type] jest ustawiane przez użytkownika, czyli może tam być cokolwiek
3) W PHP istnieją przedefiniowane stałe określające kod błędu
4) Masz takie coś jak wyjątki... dużo lepiej się sprawdza niż zwykłe true/false
5) Nazwa klasy też niezbyt dobrze trafiona.

Dobrze jest, jak na pierwszy skrypt. Nie ma sensu usuwać pól, jak dojdzie jeszcze jakaś metoda, to wtedy się przydadzą. Z tą obsługą błędów to fakt, mógłbyś lepiej to rozwiązać. Właściwie mógłbyś zwracać rezultat move_uploaded_file, przynajmniej byłoby wiadomo czy uploadowano.

Będzie m.in metoda do nazw plików.



Co proponujesz?

Typ MIME wyciągnąć z pliku dopiero po stronie serwera - a potem (tj.: fragment z in_array()) już tak samo.

W takich typu klasach, lepiej użyć __construct czy tak jak w moim przypadku?



Czyli po move_uploded_file sprawdzic mime? Jeżeli nie bedzie nieprawidłowy to co wtedy? usunać plik?

Tak, jeżeli z plikiem jest coś nie tak to usuń go.

Próbuje użyć funkcji mime_content_type, ale coś nie działa...

na localhost default nie działa, musiałem odznaczyć ";" w extension=php_mime_magic.dll

Po odznaczeniu nic nie wyświetla.

TO sprawdzaj rozszerzenie i tyle.

którą masz wersję php?? - czytaj może co piszą w manualu - te komunikaty:
mime_content_type - a dalej to masz link jakiej funkcji powinieneś użyć jeśli masz php 5.3
finfo_file

No to sie zgadza. mam wersje "PHP Version 5.2.5".

co do tego mime_type, wydaje sie jednak, że sprawdzanie rozszerzenia jest lepszym pomysłem. Co o tym sądzicie?

Troche mnie martwi fakt, ze najpeirw musi z uploadowac plik, sprawdzic mime, jeżeli sie nie zgadza to go usunac(!).

a i jeszcze "5) Nazwa klasy też niezbyt dobrze trafiona." - czemuż to?

Bo jest to bardziej logiczne i szybsze tym bardziej ze sam sie juz kiedys przejechalem na sprawdzaniu mime plikow bo nie wiedzialem ze poprzez naglowki mozna je zmodyfikowac, i staje sie to "bezpieczne" dopiero wtedy gdy go sprawdzimy jak plik jest juz na srv.

a no chyba, że tak

A nie lepiej recznie sprawdzac naglowek pliku?

Jak?

Dobra idea tego sprawdzania rozszerzenia?

[PHP] pobierz, plaintext 
class Upload{
    private $dir;
    private $type = array('jpg', 'gif', 'png');
    private $chmod = 0777;
    private $file;
 
    public function __construct($fileName, $dir){
        $this->file = $_FILES[$fileName];
        $this->dir = $dir;
 
        if(isset($this->file)){
            $nameFile = $this->createTitle($this->file['name']);
            if(!file_exists($this->dir.'/'.$nameFile)){
                if(is_uploaded_file($this->file['tmp_name'])){
 
                    if(move_uploaded_file($this->file['tmp_name'], $this->dir.'/'.$nameFile)){
                        chmod($this->dir.'/'.$this->file['name'], $this->chmod);
 
                        $d='';
 
                        $d = opendir($dir);
                        while(false !== ($f = readdir($d))) {
                            if($f == $this->file['name']){
                                $ex = explode('.', $f);
                                if(in_array($ex[1], $this->type)){
                                    //wgrano
                                }else{
                                    if(unlink($this->dir.'/'.$this->file['name'])){
                                        echo 'Zły format pliku!';
                                    }
                                }
                            }
                        }
                        closedir($d);
                    }
                }
            }else{
                echo 'Taki plik już istnieje!';
            }
        }
    }
 
    private function createTitle($title){
        $title = strtolower($title);
        $title = str_replace(' ', $this->replacechar, $title);
        return $title;
    }
}
[PHP] pobierz, plaintext 

Czy możesz napisać, co zamierzałeś zrobić, czyli po co Ci był potrzebny kod? Inaczej mówiąc - co kod miał robić (nie co robi teraz). Własnymi słowami.

Jak widać u góry dyskutowaliśmy jak sprawdzić rozszerzenie/mimetype. Wielu proponowało, aby sprawdzić rozszerzenie/mimetype po wgraniu pliku na serwer.

Dobrze to rozwiązałem?

Nie zrozumiałeś mnie


Ja bym chciał, abyś opisał to, co myślałeś zanim zacząłeś pisać kod, który zaprezentowałeś wyżej, bo to istotne.

Prosty skrypt uploadu obrazów na serwer... Sprawdzanie rozszerzenie/mime etc.

[PHP] pobierz, plaintext 
$ex = explode('.', $f);
 
if(in_array($ex[1], $this->type)){
  //wgrano
}
[PHP] pobierz, plaintext 

Zabezpieczenie żadne. Przepuści np. plik moje_zdjecia_porno.jpg.exe.

[PHP] pobierz, plaintext 
private function _checkExtension($file) {
	$temp = pathinfo($file);
	return in_array(@strtolower($temp['extension']), $this->extensions);
}
[PHP] pobierz, plaintext 

gdzie $this->extensions to tablica dozwolonych rozszerzeń, ale też zabezpieczenie żadne, bo wystarczy zmienić ręcznie rozszerzenie pliku i przejdzie.

Jak już wspomniał @phpion, sprawdzanie rozszerzenia zda się psu na budę.
Podstawową kwestią jest sprawdzenie typu mime pliku, np. w ten sposób:

[PHP] pobierz, plaintext 
$_avaiable_mimetypes = array('plain/text' , 'application/pdf' , 'application/x-pdf' , 'image/jpg' , 'image/jpeg' , 'image/png' , 'image/gif' , 'image/pjpeg' ,
        'application/vnd.ms-excel' , 'application/msword' , 'application/vnd.ms-powerpoint' );
if (!in_array($_FILES['file']['type'] , $_avaiable_mimetypes)) {
            $_errors['file_upload_error'] = '<p><span style="color:#f00;">Nieprawidłowy typ pliku.</span></p>';
        } 
[PHP] pobierz, plaintext 

PS: Kod jest żywcem wycięty, nie bawiłem się w edycję, więc proszę nie pytać, co to jest np. to: $_errors['file_upload_error']

Teoretycznie sprawdzanie typu mime też da się obejść

Oczywiście, wszystko da się obejść.
Ale nie jest to już tak trywialne, jak zmiana rozszerzenia.

Ale oczywiście, nic nie stoi na przeszkodzie, by sprawdzać również rozszerzenie. Generalnie - powinno się walidować wszystko, co się da, jeśli dopuszczamy upload plików na serwer przez "niezaufane" źródła (czyli użyszkodników)

@blooregard: typ mime z $_FILES jest ustawiany przez przeglądarkę (zobacz sobie na podgląd wysyłanych nagłówków przy pomocy Firebuga). Typ mime powinno się sprawdzać po stronie serwera.

Tak, masz rację, nie wspomniałem o tym:
http://pl.php.net/manual/pl/ref.fileinfo.php