Found an issue that broke images URL, example:
.../the_image.jpg&h=6ccb2c19?itok=oSXmdgZi As you can see the "?" is after the "?".

Correct URL:
.../the_image.jpg?h=6ccb2c19&itok=oSXmdgZi

The bug is in the implementation of hook_file_url_alter:

  // In case the image style uses "crop_type" effect, load the crop entity.
  if ($crop_type && $crop = Crop::findCrop($file_uri, $crop_type)) {
    // Found a crop for this image, append a hash of it to the URL,
    // so that browsers reload the image and CDNs and proxies can be bypassed.
    $shortened_hash = substr(md5(implode($crop->position()) . implode($crop->anchor())), 0, 8);
    $uri .= '&h='.$shortened_hash;
  }

That is adding "&h=" to the URI, if the URI do not have a "?" later another implementation of the hook can add it and broke the image.

In order to fix it we should check if there is a "?" before adding the "&".

    $uri .= (strpos($uri, '?') !== FALSE ? '&' : '?').'h='.$shortened_hash;
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

juanl created an issue. See original summary.

juanl’s picture

juanl’s picture

Assigned: juanl » Unassigned
joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

That seems reasonaable, the whitespace around the periods needs to be added for coding standards but that could be fixed on commit. There is probably some PHP or drupal helper methods for building a URL out but don't have them on hand so this seems reasonable solution.

woprrr’s picture

Hi all I review it and merge it fast.

weri’s picture

woprrr’s picture

Status: Reviewed & tested by the community » Closed (cannot reproduce)

I can't reproduce this bug in current state of Crop API module because I guess berdir patch already fixe that #2868339: Public folder check in crop_file_url_alter() is incorrect, does not work for responsive images.

Example :

Fell free to switch status of issue if your problem persist but I think all works as excepted now.