the crop_file_url_alter() is looking for a crop_type key in the image style effect configuration, but for a module provided crop type that extends ResizeImageEffect this is not a default key in the effect's configuration. Focal Point for example provides a crop that does not store the crop type in the image style configuration. Using the provider as a fallback we can check if there's a crop type with the provider name to set the value for passing to Crop::findCrop().

The attached patch does this.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

blake.thompson created an issue. See original summary.

blake.thompson’s picture

Issue summary: View changes
woprrr’s picture

Category: Bug report » Support request
FileSize
3.6 KB

Hii @blake.thompson

Thank for this feedback. We can't consider that issue "Bug report" but more "Support Request" because in this case we need to add fallback for provider not using usual way (using manual crop effect) but using crop entity without his provided effect. This case seem's to be a little
less efficient than without fallback because we try to predict if providers match with crop_type and that tests fire a lot when we try to display an image using an ImageStyle.

I just process to a little extract method to add getCropFromImageStyle() at crop basis because that is really usefull for provider to evaluate if a crop match with a crop_type directly with combo uri / image_style and that permit to reduce the general complexity of hook in crop.module.

Please can you tell me if that works with your usecases/providers @blake.thompson

blake.thompson’s picture

So sorry, I finally got around to testing your patch. It looks like it does solve the issue if a crop type is not setting that value in its configuration. It looks like this issue was tackled within the Focal Point project as well https://www.drupal.org/node/2842260. Perhaps it just needs to be up to the module using Crop API to implement it correctly? At the same time it shouldn't hurt to have a fallback.

blake.thompson’s picture

Status: Needs review » Reviewed & tested by the community

  • woprrr committed 6447090 on 8.x-1.x
    Issue #2899173 by blake.thompson, woprrr: crop_file_url_alter doesn't...

  • woprrr committed 275f8ae on 8.x-2.x
    Issue #2899173 by blake.thompson, woprrr: crop_file_url_alter doesn't...
woprrr’s picture

Status: Reviewed & tested by the community » Fixed

Merged Thanks all :) good job.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.