Split ImageStyle into the config entity and a separate event-based image processing service

The Needs Review Queue Bot → tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Rebased.

cspell error

/var/www/html/core/modules/user/tests/src/Functional/UserPictureTest.php:123:34 - Unknown word (getfile)

rebased onto 11.x

Seems like something that could use a change record.

For any contrib modules that add an image style will they have to do anything or will this work automatically.

@smustgrave re #81:

Seems like something that could use a change record.

For sure. However, 5 years after this was started, there seems to be little or no interest to move this forward - at least I cannot see reviews in this sense. So, either we simply just won't fix this, or a review in the sense of agreeing on the architecture (or a propositive disagreement) is needed. Writing a CR now would be simply too early/useless. Please leave to NR while we get a review - or close this.

For any contrib modules that add an image style will they have to do anything or will this work automatically.

Not sure I understand. You can add image effect plugins. Or configuration of image styles. Both are not affected by this.

Did a first pass of review and wondered do we really need so many events fired while generation happening.

Very probably it affects performance as each event is dispatched (as I got).

Maybe instead it could use less events as some events can carry state of process

For #83

I give up, sorry.

I could think of doing something in contrib, but core should at a minimum split the entity from the image processing code - in a way that it could be overridable. AFAICU it's not possible at the moment.

Wasn't trying to shoot down the idea. Not something I can speak heavily on though.

No hard feelings, @smustgrave. Just I think after 5 years being open, there's no community interest at carrying this on. So I won't spend time on rerolls.

I really don't think this should be closed.

I have been a bit booked by other tasks so I haven't been able to give it a test run, which I really wanted to do before commenting to hopefully better comment, but given there is a move to close it I feel a need to comment without having done the testing yet.

@andypost in #83 you assert there is a 'probable' performance impact but did not provide a profile report with your post? Can you please attach the generated performance report file for the rest of us to review?

While I've not traced every single event yet in this MR, the majority of the ones I have are only needed when an image derivative doesn't exist, the majority of times images are served from the file on disk. The most common to trigger event I would expect to be the buildUrl() events which need to be fired to generate links to ImageStyles Derivatives, though the flexibility of an event is necessary to allow all the changes that have been requested over the years.

Could some events be combined? Without looking at every line, Maybe? But would it make sense? Most of these events are in response to a specific method being called, if no-one ever calls that method, why invoke the expense of gathering data that no-one will use?

IMHO it would be a shame for this work to be lost especially since #83 didn't provide any objective data to justify moving to needs-work.

larowlan → closed merge request !105

Closed the outdated MR

Looking at the patch next

Left some comments on the MR

Keeping at NR and retaining the 'Needs architectural review' tag

Added 📌 [policy, no patch] Decide on policy regarding use of Events class with string constants for event names or class names instead Active for discussion

Thanks @larowlan for the review - I'll address your points but it may take a while.

Replied to @larowlan's comments, inline.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

@mondrake, deviantintegral, @bradjones1 thanks a lot for your work!

Just I think after 5 years being open, there's no community interest at carrying this on. So I won't spend time on rerolls.

I'm working on a way of bringing UI to specify image styles beeing used for remote images for IntelligenceBank DAM → and I think this issue is a good base for it. Typically DAMs like IB or PicturePark provides a way to generate image derivatives on their side, but there is a lack for selecting right image style in content context (like ckeditor, specific node, block). In Drupal we have many ways to map content item with right display settings, but it fails if image is remote and we don't need generate derivative ourselfs.

I will check this issue more deeply!