Problem/Motivation

Image.module has two remaining functions and a constant.
Let's deprecate them so we can remove the file in 13.x

Steps to reproduce

Open file

Proposed resolution

Create service for image_path_flush and image_style_options
Add TOKEN constant to class. This cannot be an enum since it is used in an array.

Remaining tasks

Review

User interface changes

N/A

Introduced terminology

N/A

API changes

New ImageDerivativeUtilities service

Data model changes

N/A

Release notes snippet

N/A

Issue fork drupal-3567618

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

nicxvan created an issue. See original summary.

nicxvan’s picture

nicxvan commented
Issue summary: View changes

nicxvan opened merge request !14372

nicxvan’s picture

nicxvan commented
Issue summary: View changes
nicxvan’s picture

nicxvan commented
Status: Active » Needs review

I think this is ready.

I went back and forth on whether it should be one service or several, but it's pretty small.

I can update the name if someone comes up with a better one.

I think plugin constructors don't need anything special for BC: https://www.drupal.org/about/core/policies/core-change-policies/bc-polic...

nicxvan’s picture

nicxvan commented
Title: Create service for image_path_flush, image_style_options and enum for IMAGE_DERIVATIVE_TOKEN » Create service for image_path_flush, image_style_options and constant for IMAGE_DERIVATIVE_TOKEN
nicxvan’s picture

nicxvan commented
Version: 11.x-dev » main
dcam’s picture

dcam commented

I think IMAGE_DERIVATIVE_TOKEN might be better moved to core/modules/image/src/Entity/ImageStyle.php. I considered ImageStyleDownloadController. But ImageStyle is where the token is used to build style URLs. That seems like the most appropriate place to me.

I'm still analyzing what you said in #5.

nicxvan’s picture

nicxvan commented

It feels wrong in either place for the constant, since both are pretty heavy classes.

If someone else wanted to use that constant I don't think we would want to require them to include ImageStyle or ImageStyleDownloadController?

nicxvan’s picture

nicxvan commented
Issue summary: View changes
dcam’s picture

dcam commented
Status: Needs review » Needs work

Setting to Needs Work for a rebase.

If someone else wanted to use that constant I don't think we would want to require them to include ImageStyle or ImageStyleDownloadController?

That's fair. The only other suggestion I might have would be a single-case backed enum like the one proposed for the _none option. But I really don't care that much. Part of me just feels weird including the constant in a service class that doesn't use it. But there's no way I'm dying on this hill. It isn't worth arguing over. I guess you could say I'm just brainstorming.

For the record, the backed enum idea has no precedent in core. I searched for other uses of query->get\(. Nearly all query parameter names in get() are written out there. There's one parameter name stored in a constant, MainContentViewSubscriber::WRAPPER_FORMAT. That isn't to say we couldn't do it. It just means no one else ever has. And there are also no single-case enums in core right now. I checked for that too. I don't know how much support that idea would get. And you'd have to get the case's value everywhere, unlike with a constant. So I admit it isn't necessarily a good idea.

Anyway, if you want to leave it as-is, then say so and I'll RTBC it after the rebase.

nicxvan’s picture

nicxvan commented

Honestly I don't feel super strongly about this myself, now that I think about it, we can put it on the ImageStyleInterface, that is already included in the controller.

A single case enum felt wrong too, I think I'll move it to the interface and see how that looks after rebasing.

nicxvan’s picture

nicxvan commented
Status: Needs work » Needs review

How does this look?

dcam’s picture

dcam commented
Status: Needs review » Needs work

Yeah, I like it. I never think about adding constants to interfaces. Let's go with this.

I found a typo on my final review. Also, the constant wasn't removed from the service. Then we'll be finished with it.

nicxvan’s picture

nicxvan commented
Related issues: +#3575042: Fix antipattern of injecting image_style storage directly

I applied both suggestions thank you!

I also created this follow up #3575042: [pp-1] Fix antipattern of injecting image_style storage directly

nicxvan’s picture

nicxvan commented
Status: Needs work » Needs review
dcam’s picture

dcam commented
Status: Needs review » Reviewed & tested by the community

Looks good to me. Thank you for considering my feedback.

claudiu.cristea made their first commit to this issue’s fork.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented

Straight reroll ^

godotislate’s picture

godotislate
he/him
commented
Status: Reviewed & tested by the community » Needs work

Some comments on the MR.

godotislate’s picture

godotislate
he/him
commented
Issue tags: +Needs change record updates

Almost forgot to mention that the CR could use a little editing for grammar in the last two lines.

nicxvan’s picture

nicxvan commented
Status: Needs work » Needs review

I edited the CR and resolved all comments except for one that I'd like to do in a follow up. I'll create that once you confirm.

I'll keep an eye on tests.

nicxvan’s picture

nicxvan commented

Ah I missed @godotislate's response on the mr ticket is here #3580702: [pp-1] update dependency injection for image widget

This is ready again.

dcam’s picture

dcam commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs change record updates

I read through the recent comments since my last review, both here and in the MR. All feedback has been addressed. I did an additional code review of the changes and didn't find anything else to comment on. The change record was updated, but my personal preference is to show calling the service when introducing one, so I updated the code examples to show that. I hope you don't mind.

amateescu made their first commit to this issue’s fork.

  • amateescu committed 54c6c2e5 on main 
    task: #3567618 Create service for image_path_flush, image_style_options...
amateescu’s picture

amateescu commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Found a few minor points that I fixed myself with code suggestions, otherwise this looks great.

Committed 54c6c2e and pushed to main. Thanks!

Needs a 11.x backport MR though...

claudiu.cristea opened merge request !15378

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Status: Patch (to be ported) » Needs review

Created a new MR. Let's see the bot

claudiu.cristea opened merge request !15379

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
  • MR 15379: followup to remove the constant in 13.0.0, rather than 12
  • MR 15378: 11.x version
claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented

Moved the removal to 13.0.0 also for the functions

amateescu’s picture

amateescu commented
Status: Needs review » Reviewed & tested by the community

Everything looks great now, let's wait for the pipelines to finish :)

  • amateescu committed c6a71ff4 on main 
    task: #3567618 followup - Bump deprecation removals to Drupal 13

By:...

  • amateescu committed 28426e34 on 11.x 
    task: #3567618 Create service for image_path_flush, image_style_options...
amateescu’s picture

amateescu commented
Status: Reviewed & tested by the community » Fixed

Committed c6a71ff and pushed the followup MR to main, and 28426e3 to 11.x. Thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

amateescu closed merge request !15378

amateescu closed merge request !15379

amateescu closed merge request !14372

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented

Published the CR https://www.drupal.org/node/3567619