Contrib.social

Remove the cases specific to the crop_crop effect plugin and replace them with general management of effect plugins.

Open on Drupal.org →
Created on 30 July 2024, 6 months ago

Problem/Motivation

In the CROP entity code, the use of the ‘crop_crop’ effect plugin is hard-coded at several key points in the process. Introducing complexity into the code, it takes the form of : 

if(is_crop_crop) {
 do something
}
else {
 do something, but almost the same
}

The main problem this creates is that for retrieving the Crop Type in the ‘getCropFromImageStyleId’ method, only the ‘crop_crop’ plugin is allowed to have a selection of the Crop type, in all other cases the ‘Crop type’ must correspond to the Provider (the module) declaring the effect plugin.

Steps to reproduce

NA

Proposed resolution

It would be preferable to do away with these special cases and use the crop_crop plugin as the basis/template for implementing other CROP-based effect plugins.

Remaining tasks

Documentation :
- Specify that any effect plugin wishing to allow selection of the crop type must store this value in a storage named ‘crop_type’.

User interface changes

NA

API changes

All

Data model changes

NA

📌 Task
Status

Active

Version

2.0

Component

Code

Created by

🇫🇷France DrDam

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @DrDam
  • Comment 6 months ago →
    🇫🇷France DrDam
  • Status changed to Needs review 6 months ago
  • Comment 6 months ago →
    🇫🇷France DrDam
  • Merge request !17fix #3464677 : Provide generic way to use effect set crop_type → (Open) created by DrDam
  • Pipeline finished with Success
    4 months ago
    Total: 143s
    #273141
  • Pipeline finished with Success
    4 months ago
    Total: 147s
    #273142
  • Status changed to RTBC 4 months ago
  • Comment 4 months ago →
    🇫🇷France DrDam
  • Status changed to Needs review about 2 months ago
  • Comment about 2 months ago →
    🇷🇺Russia zniki.ru

    Thanks for your MR, but I believe this issue still need review.
    I hide patch file, because MR approach used.

    There is 2 issue related on this topic.
    And at first sight it looks like methods: getCropFromImageStyle, getCropFromImageStyleId, cropExists, findCrop should be moved away from CropInterface. Maybe we need to make bigger refactor to impove developers expirience.

  • Comment about 1 month ago →
    🇫🇷France DrDam

    And at first sight it looks like methods: getCropFromImageStyle, getCropFromImageStyleId, cropExists, findCrop should be moved away from CropInterface. Maybe we need to make bigger refactor to impove developers expirience.

    I totaly agree, but without knowing the history of the implementation, it's not clear how far the refactor can go.

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024