Return to Revisions

The feedback below won't likely prevent time out errors, but hopefully will clean up the code.

###Description-code mismatch

The narrative in your post states:

Basically this is what it does :

• checks if the images exist. If it does it generates the <picture> element.

But the first few lines of the function are:

public static function picturebuilder($imgUrl, $imageAlt, $params) {

    // check if the image exists
    if (!file_exists($imgUrl)) {
      echo "<script>console.log( 'Cannot find : " . $imgUrl . "' );</script>";
      return false;
    }

So the code appears to actually add a JavaScript console.log() call on the client side, instead of adding a <picture> element...

###Outputting JavaScript with echo If you are going to add JavaScript to the client side and echo the image path (i.e. $imgUrl) it would be wise to sanitize that value so it won't break the JavaScript. One technique for that is to use json_encode() or htmlspecialchars().

echo "<script>console.log( 'Cannot find : " . htmlspecialchars($imgUrl) . "' );</script>";

You didn't specify which file system the files are coming from, but some systems allow single-quotes and semi-colons in file names (e.g. the MS NT file system - refer to this answer for a summary).

So it may seem unlikely but possibly that a filename like the following would be passed:

$imgUrl = "photo1';compressed.png";

See how well that works with the echo statement from your code in this playground example. In the browser console, there should be an error:

Uncaught SyntaxError: missing ) after argument list

###incrementing a counter variable inside a foreach The foreach statement has "two syntaxes"¹ and because the array indexes of $thumbWidths are numeric, the second one can be used to have the value of $i updated automatically (i.e. to have each numeric index assigned , starting at 0), reducing the initial assignment of the counter variable (i.e. $i = 0;) and updating the counter value at the end of each block (i.e. $i++;).

foreach (array_expression as $key => $value)

So blocks like this:

$i = 0;

$thumbPaths = array();
foreach ($thumbWidths as $thumbWidth) {
  $thumbHeight = $thumbHeights[$i];
  $thumbPath = $thumbDir . '/' . $imgName . '-' . $thumbWidth . 'x' . $thumbHeight . 'q' . $thumbQuality . $imgExtension;
  array_push($thumbPaths,$thumbPath);
  $i++;
}

Can be simplified like so:

$thumbPaths = array();
foreach ($thumbWidths as $i => $thumbWidth) {
  $thumbHeight = $thumbHeights[$i];
  $thumbPath = $thumbDir . '/' . $imgName . '-' . $thumbWidth . 'x' . $thumbHeight . 'q' . $thumbQuality . $imgExtension;
  array_push($thumbPaths,$thumbPath);
}

And array_map() could also allow it to be simplified even further, though the variables outside the foreach loop would have to be imported into the callback function with the use statement, which might seem a bit wonky:

$prefix = $thumbDir . '/' . $imgName . '-';
$suffix = 'q' . $thumbQuality . $imgExtension
$thumbPaths = array_map(function($thumbWidth, $i) use ($thumbHeights, $prefix, $suffix) {
    $thumbHeight = $thumbHeights[$i];
    return $prefix . $thumbWidth . 'x' . $thumbHeight . $suffix;
}, $thumbWidths, array_keys($thumbWidths));

###Repeated code when writing images The code in the if-else blocks near the end is very identical... the only thing that changes is if $crop evaluates to true then there is a call to cropThumbnailImage(), otherwise resizeImage() is called. Outside of that, the rest of the code appears identical:

if($crop) {

    foreach ($thumbWidths as $thumbWidth) {
      $imagick = new imagick($imgUrl);
      $thumbHeight = $thumbHeights[$i];
      $imagick->cropThumbnailImage($thumbWidth,$thumbHeight);
      $imagick->setImageCompressionQuality($thumbQuality);
      $imagick->writeImage($thumbPaths[$i]);
      $imagick->clear();
      $imagick->destroy();
      $i++;
    }
  } else {
    foreach ($thumbWidths as $thumbWidth) {
      $imagick = new imagick($imgUrl);
      $thumbHeight = $thumbHeights[$i];
      $imagick->resizeImage($thumbWidth,$thumbHeight,imagick::FILTER_LANCZOS, 1, true);
      $imagick->setImageCompressionQuality($thumbQuality);
      $imagick->writeImage($thumbPaths[$i]);
      $imagick->clear();
      $imagick->destroy();
      $i++;
    }
  }

So that could be simplified to the following (also uses the numerical-index array syntax - see above):

foreach ($thumbWidths as $i => $thumbWidth) {
    $imagick = new imagick($imgUrl);
    $thumbHeight = $thumbHeights[$i];
    if ($crop) {
        $imagick->cropThumbnailImage($thumbWidth,$thumbHeight);
    }
    else {
        $imagick->resizeImage($thumbWidth,$thumbHeight,imagick::FILTER_LANCZOS, 1, true);
    }
    $imagick->setImageCompressionQuality($thumbQuality);
    $imagick->writeImage($thumbPaths[$i]);
    $imagick->clear();
    $imagick->destroy();
}