1. PHP / Говнокод #3292

    +165

    1. 1
    2. 2
    3. 3
    4. 4
    5. 5
    $image = $_FILES['image']['tmp_name'];    
    if( !empty($image) )
    {
       @$src = imagecreatefromjpeg($image);
if($src==false){exit ('Это не картинка'); }

    Этот кусочек кода был спором 7 человек;)
    Это говнокод?

    Запостил: FallenServer, 22 Мая 2010

    Комментарии (40) RSS

    • ava guest 22.05.2010 10:47 # −5

      показать все, что скрытоЭто не картинка.
      Ответить

    • ava guest 22.05.2010 11:16 # −6

      показать все, что скрытоСразу видно, что не MVC => говно by design
      Ответить

    • ava guest 22.05.2010 12:50 # +6

      Я, guest, находясь в нетрезвом состоянии, торжественно заявляю:
      Есть функция getimagesize() которая выводит FALSE если пикча инвалидная
      Ответить

    • ava striker 22.05.2010 13:50 # +6

      @ уже означает говнокод
      Ответить

      • ava guest 22.05.2010 14:31 # +3

        Не соглашусь. Бывают ситуации, когда вывода теоретически возможного сообщения об ошибке нужно по-любому избежать, даром что обработку ошибки ты предусматриваешь. Сермяжный пример: $dbLink = @mysql_connect(...). (Про PDO, конверсию ошибок в исключения, абстракцию от БД и прочие true coding practices в курсе, но это не значит, что я вижу смысл абсолютно всегда ими пользоваться.) Другой вариант - @unlink(), если мы не уверены, что удалить файл удастся (safe_mode иногда может не приниматься в расчёт функцией is_writable()). Ещё вариант - @unserialize(), когда мы не уверены, что данные приходят из надёжного источника (например, из cookies) - не городить же валидацию сериализованных данных. Хотя всё это, конечно же, не отменяет необходимости стремиться избежать использования @ насколько это только возможно.
        Ответить

        • ava striker 22.05.2010 14:41 # +3

          в пхп можно настроить отлов всех типов ошибок (warning, notice, etc) и кидать вместо них исключения
          в том же мзз unlink выглядит так
          try {
          unlink('file.txt');
          } catch (mzzException) {
          echo 'Ошибка удаления файла';
          }
          Ответить

          • ava guest 25.05.2010 20:04 # 0

            try-catch блоки кидать везде подряд - тоже безвкусица и говнокод
            Ответить

            • ava striker 26.05.2010 00:42 # 0

              везде, где он может быть кинут - не говнокод
              Ответить

        • ava striker 22.05.2010 14:45 # 0

          Но вообще, ваша позиция мне знакома, поэтому плюсанул вам каммент
          Ответить

        • ava wwwguru 24.05.2010 12:38 # 0

          Можно даже без try .. catch - просто своя функция в set_error_handler()
          Ответить

    • ava guest 23.05.2010 15:56 # −1

      если в tmp_name содержится расширение (не вспомню точно на самом деле)), то лучше бы сначала проверить на предмет оного, чем сразу-же напрягать сервер вызовом imagecreatefromjpeg.
      Ну а во-вторых - форматирование кода ни к черту, но блиадь, кучу одноразовых переменных - объявим. Зачем? Что-бы легче свой кал понять можно было?
      На говнокод не тянет, но на никчемный калокод - еще как.
      Ответить

      • ava cheef 24.05.2010 12:01 # +2

        и что тебе даст это расширение?
        Ответить

        • ava turdman 24.05.2010 13:56 # 0

          Отфильтровать очевидные ошибки пользователя.
          Хотя практическая польза, скорее всего, будет невелика
          Ответить

          • ava cheef 24.05.2010 14:04 # +1

            Это пустая трата времени, так-как совсем ненадёжно. Проще сразу же проверять mime-type и не париться
            Ответить

            • ava Lure Of Chaos 24.05.2010 20:12 # 0

              однопенисуально, что расширение, что миме, которое от броузера пришло - все равно не гарантия, что картинка
              Ответить

              • ava cheef 25.05.2010 12:40 # 0

                причём тут finfo и " расширение, что миме, которое от броузера пришло"? Почитайте документацию.

                Вы случаем на пол-ставки КО не подрабатываете? :) Очевиден факт, что можно подделать расширение, заголовки, в том числе и файла. Но наиболее приемлемый уровень надёжности обеспечивает проверка заголовков файла, её достаточно во всех случаях, кроме исключительно специфичных - когда программист параноик :) А остальные решения вполне можно причислить к велосипедам.
                Ответить

    • ava guest 23.05.2010 16:13 # +8

      Этот код, безусловно, говно в силу следующих причин:
      1) Множество кодов на PHP является подмножеством говнокодов
      2) Парсинг параметров и работа с картинками не должна делатся в одном месте: это разные задачи, и они должны быть разнесены в разные классы иначе код не покрыть юнит-тестами, а так же получается low cohesion
      3) затыкание чего либо собачкой (равно как и использование ERROR_LEVEL ниже DEBUG) -- дурной тон и прямой путь к ошибкам
      4) использование "exit" не позволяет использовать этот модуль в других задачах: библиотека не имеет права останавливать работу приложения.
      5) Хардкод литералов не позволяет нормально локализовать приложение.

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

      Благодарю тебя Господи, что я не PHP программист.
      Ответить

      • ava telnet 23.05.2010 19:18 # +1

        Хорошо сказал, всё по делу. Вот только
        > 1) Множество кодов на PHP является подмножеством говнокодов
        Не всякий код на PHP - говнокод. Я согласен, что из-за низкого порога вхождения и косяков в, скажем так, идеологии языка писать на PHP говнокод во сто крат легче и проще, чем нормальный чистый код, и даже есть искушение писать как проще, а не как правильнее. Я прекрасно вижу, как пагубно сии факты влияют на профессиональные качества программистов, как тормозят их рост, а в клинических случаях и вовсе его стопорят ("знаешь что, иди-ка ты на хрен со своими парадигмами, я так привык/завтра заказ сдавать..."). Но это не означает, что нормальный чистый код на PHP а) написать невозможно и б) никто никогда не пишет. Хотя тот факт, что отношение обезьян к людям среди PHPшников заметно выше, чем среди пишущих на других языках, самого очень расстраивает.
        Ответить

        • ava Lure Of Chaos 23.05.2010 22:28 # +1

          наверное все дело в принципах и возможностях языка... Чем строже язык, тем меньше говна - я выделяю c,с# и java
          Я представляю какое вонючее говно можно было бы написать на том же Katahdin
          Ответить

          • ava telnet 24.05.2010 05:33 # 0

            Да, пожалуй. Но в случае с PHP клиника ещё и в том, что многие идеологически неправильные принципы вводились в него искусственно, причём подчас с благими намерениями. Банальнейший пример тому - magic_quotes: официальное его назначение - помочь обезьянам, забывающим экранировать входящие данные при подстановке в SQL-запросы и файловые функции, сделать их быдлоскрипты безопаснее. В итоге сильно безопаснее скрипты таки не стали, зато всем пришлось плеваться и очищать данные от лишних слэшей, а затем, когда magic_quotes стало можно выключать, писать костыли для совместимости со включённым и выключенным режимами. Маразм чистейший, но скоро вот отмечаем десять лет, как все с ним трахаются.
            Ответить

            • ava cheef 24.05.2010 12:11 # +2

              magic_quotes, кстати, уже deprecated
              Ответить

            • ava Lure Of Chaos 24.05.2010 17:38 # −1

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

    • ava redh3ad 23.05.2010 23:19 # −2

      >Есть функция getimagesize() которая выводит FALSE если пикча инвалидная +1

      В любом случае:
      $type = image_type_to_mime_type($image);
 if ($type !== 'IMAGETYPE_JPEG') {
     die('Это не картинка');
 } else {
     ...
 }
      Ответить

      • ava This is obvious 24.05.2010 09:28 # +1

        по твоему *.png, *.gif, *.bmp не картинки?
        Ответить

        • ava mrbig66 24.05.2010 10:48 # 0

          Зависит от задумки автора. Вы же не хотите, чтобы пользователи начали загружать на ваш сайт исключительно бээмпэшки.
          Ответить

        • ava redh3ad 24.05.2010 12:00 # +1

          У ОПа:
          @$src = imagecreatefromjpeg($image);

          Естествеено, по-хорошему проверка на все типы изображений.
          Ответить

        • ava Lure Of Chaos 24.05.2010 20:35 # −1

          tga забыли ))
          Ответить

      • ava telnet 24.05.2010 11:46 # 0

        Что-то тут напутано. По мануалу, image_type_to_mime_type() возвращает MIME-тип (в данном случае "image/jpeg"), а на вход мы ей кормим число, равное одной из констант IMAGETYPE_xxx. По мануалу же, это число можно взять из возврата функции getimagesize() (элемент массива с индексом 2). Правда, из возврата той же функции можно сразу взять MIME-тип (элемент массива с символьным ключом 'mime').
        Ответить

        • ava redh3ad 24.05.2010 12:07 # −1

          Да, каюсь, напутал :)
          ...
$properties = getimagesize($file)
    or die('Это не картинка');
switch ($properties[2]) {
    case IMAGETYPE_JPEG:
        $src = imagecreatefromjpeg($file);
    break;
    case IMAGETYPE_GIF:
        $src = imagecreatefromgif($file); 
    break;
    case IMAGETYPE_PNG:
        $src = imagecreatefrompng($file); 
    break;
    ...
    ...
}
          Ответить

          • ava This is obvious 24.05.2010 13:41 # +3 

            $props = getimagesize($file)
while(!$props)
{ 
          die; 
          if(!die){
                      exit; 
           } else if (!die && !exit) 
           { 
                          do
                          {
                            die; exit;
                           } while(!die && !exit);
           }
}
            Ответить

            • ava This is obvious 24.05.2010 13:47 # −1

              подход реальных программиздов.
              Ответить

            • ava telnet 24.05.2010 14:09 # +6

              Первые две строчки должны быть спеты нежным женским голосом, остальное гроулом под звуки расстроенной электрогитары, визг болгарки, скрежет напильника и стоны забиваемого скота. Сингл назовём "Узнай размеры картинки или умри" ("Get image size or die").
              Ответить

            • ava mrbig66 24.05.2010 14:11 # −1

              +1000
              Ответить

    • ava cheef 24.05.2010 12:10 # −1

      вообще действительно приличной реализацией этой задачи была бы проверка mime-type, через тот же finfo
      Ответить

      • ava telnet 24.05.2010 12:20 # −1

        > PHP >= 5.3.0, PECL fileinfo >= 0.1.0
        Пока ещё не кроссхостингово. Кто как, а я пока не рискую пользоваться новыми фишками 5.3. Вот пошире распространится...
        Ответить

        • ava cheef 24.05.2010 12:23 # −1

          Это да, но я использую версию из PECL с php < 5.3. Оказалось, что проще установить эту либу на продакшене, чем изобретать велосипеды.
          Ответить

      • ava HyperGeek 24.05.2010 15:37 # −2

        Ну или ImageMagick, он более распространен. К тому же getimagesize() не распознает некоторые JPEGи (может уже пофиксили).
        Ответить

    Добавить комментарий