Problem/Motivation

ImageStyle::getReplacementID is written like its implementing an interface but it doesn't exist on the interface:


  /**
   * {@inheritdoc}
   */
  public function getReplacementID() {

This means the method doesn't actually have any documentation.

Steps to reproduce

Code issue. You'll have to look at ImageStyle.php and ImageStyleInterface.php or see the missing hints in your IDE.

Proposed resolution

Deprecate the method for removal because no usages of it can be found anywhere.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3498540

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

neclimdul created an issue. See original summary.

neclimdul’s picture

neclimdul
He/Him
commented

This actually shows up in the phpstan report as well.

I think this might actually be a dead method as well so removing it might also be an option. It just proxies through to the storage method of the similar name(different capitalization) and using that might be the correct way of accomplishing this.

quietone’s picture

quietone commented
Version: 11.1.x-dev » 11.x-dev

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

shalini_jha opened merge request !10902

neclimdul’s picture

neclimdul
He/Him
commented
Status: Active » Reviewed & tested by the community

Looks good for the straightforward fix.

Will leave questions of the not strictly camelcacse code and possible code removal to committer review and RTBC this.

quietone’s picture

quietone commented

If it really isn't used we should deprecate it now instead of postponing that..

I didn't find usages in core. I didn't find one in contrib either. Did I miss a usage? Can someone check on that?

shalini_jha’s picture

shalini_jha commented

@quietone You are correct. I have also checked the usage of ImageStyle::getReplacementID, and I could not find any.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

I think @quietone is right and we should deprecate this instead.

shalini_jha’s picture

shalini_jha commented
Status: Needs work » Needs review

I’ve deprecated the getReplacementID() method after confirming it isn’t used in core or contrib, and I followed a similar approach to other deprecated methods. Moving this NR , Could you please review and let me know if anything needs to be updated or improved?

Thank you for the guidance!

smustgrave’s picture

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

Feedback appears to be addressed.

shalini_jha’s picture

shalini_jha commented

Thank you for the feedback , yes {@inheritDoc} is there so no need to add the deprecation here , i have checked other deprecated method also for same with {@inheritDoc}. i have updated here also.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

The change record is incorrect here - it needs to talk about the method being added to the interface, not the deprecation that was removed from the MR.

neclimdul’s picture

neclimdul
He/Him
commented
Status: Needs work » Needs review

Talked to catch to clarify and we don't really need to push the method to the interface at all since this is dead code and it just adds complexity.

I shuffled Shalini's code around to accomplish this. Also bumped code and CR to 11.3 as well as point the unlikely user that finds this to the method on the storage class that still has this functionality.

catch’s picture

catch
he/him
Primary language English
commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Looks good and the CR lines up.

Updated the proposed resolution which was still out of date.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Reviewed & tested by the community » Needs work

Shouldn’t this have a deprecation @trigger_error anyway?

catch’s picture

catch
he/him
Primary language English
commented
Title: ImageStyle::getReplacementID missing from interface » Deprecate unused ImageStyle::getReplacementID()

It should. Also updating the issue title for what's actually happening.

shalini_jha’s picture

shalini_jha commented
Status: Needs work » Needs review

I’ve addressed the feedback related to @trigger_error. Moving this back to Needs Review. Please review and let me know if any further updates are required.

dcam’s picture

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

This looks great. The issue summary contained all the information I needed to understand the issue. The edited docblock contains correct information now. The change record URLs point to the correct place. I edited the CR a little to fix a typo and add before/after examples.

We often use @trigger_error(__METHOD__ . "() is deprecated..."); when deprecating a class method so its FQCN and name are dynamically included. But I don't consider this to be a reason to set the issue back to Needs Work status. I'm only mentioning it so people can learn. I wouldn't make the change unless someone insists on it.

  • catch committed 9ba7f47e on 11.3.x 
    task: #3498540 Deprecate unused ImageStyle::getReplacementID()

By:...

  • catch committed f7b1c2b9 on 11.x 
    task: #3498540 Deprecate unused ImageStyle::getReplacementID()

By:...
catch’s picture

catch
he/him
Primary language English
commented
Version: 11.x-dev » 11.3.x-dev
Status: Reviewed & tested by the community » Fixed

I looked for why this was unused, using git log -S getReplacementId, it was originally deprecated in Drupal 8.0.0, but then didn't get properly removed #2479487: ImageStyles can be deleted while having dependent configuration..

Since this is long dead code, I went ahead and cherry-picked to 11.3.x rather than updating the deprecation to 11.4.x.

Committed/pushed to 11.x and cherry-picked to 11.3.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.

catch closed merge request !10902

Status: Fixed » Closed (fixed)

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