Updated: Comment #0

Part of #2105863: [meta] Images, toolkits and operations. This was stated first time in #2103635-21: Remove effects from ImageInterface.

Problem/Motivation

Image processing has now several layers of abstraction:

Level Class Effect/operation method Abstraction
Image style ImageEffectInterface applyEffect() Toolkit agnostic
Image ImageInterface apply() Toolkit agnostic
Toolkit ImageToolkitInterface apply() Toolkit dependent
Toolkit operation ImageToolkitOperationInterface apply() Toolkit dependent

The higher image style level has a different naming from the others.

Proposed resolution

Unify naming by renaming ImageEffectInterface::applyEffect() to ImageEffectInterface::apply().

Remaining tasks

N/A

User interface changes

No changes.

API changes

Renamed ImageEffectInterface::applyEffect() to ImageEffectInterface::apply().

CommentFileSizeAuthor
#46 interdiff_25_45.txt9.21 KBprem suthar
#45 2109343-45.patch8.31 KBprem suthar
#25 unify-image-effect-naming-2109343-25.patch9.14 KBmandar.harkare
#21 unify-image-effect-naming-2109343-21.patch6.61 KBmandar.harkare
#20 Unify-effect-operation-verb-naming-in-image-style-2109343-20.patch7.81 KBStudiographene
#16 rename-method-2109343-15.patch7.5 KBErrolSilver
#10 rename-method-2109343-10.patch7.8 KBErrolSilver
#5 rename-method-2109343-5.patch8.09 KBsumeet.pareek
#1 rename-method-2109343-1.patch7.33 KBsumeet.pareek

Issue fork drupal-2109343

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

sumeet.pareek’s picture

sumeet.pareek commented
Issue summary: View changes
Status: Active » Needs review
Issue tags: +DrupalWorkParty
StatusFileSize
new rename-method-2109343-1.patch7.33 KB

I think this simple patch should do it.

yesct’s picture

yesct commented

@Sumeet.Pareek did you do this using an IDE with a refactor function, or grepping and doing other unix string replacements?

I'm wondering if we can give a hint for how someone could review this. maybe by grepping for any remaining "applyEffect"

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented

@Sumeet.Pareek thank you for contributing.

@YesCT will review as soon it turns green. Thank you!

Status: Needs review » Needs work

The last submitted patch, 1: rename-method-2109343-1.patch, failed testing.

sumeet.pareek’s picture

sumeet.pareek commented
Status: Needs work » Needs review
StatusFileSize
new rename-method-2109343-5.patch8.09 KB

@YesCT, @claudiu.cristea - Being new to D8 contrib, this is what I did -

  1. Tried understanding this issue and the parent meta issue #2105863: [meta] Images, toolkits and operations
  2. grep-ed recursively in core/modules/image/ for 'applyEffect' and read through all the files that showed up.. (and also some others that did not show up.. because things started to make sense) :-)
  3. I figured a standard 'replace all' from `applyEffect` to `apply` is all that is needed here, and did that using sublime text where I have D8 checkout as a project now.

Looking at the failed test results, I see that I missed renaming at one occasion 

Fatal error: Class Drupal\image_module_test\Plugin\ImageEffect\NullTestImageEffect contains 1 abstract method and must therefore be declared abstract or implement the remaining methods (Drupal\image\ImageEffectInterface::apply) in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/image/tests/modules/image_module_test/lib/Drupal/image_module_test/Plugin/ImageEffect/NullTestImageEffect.php on line 30
FATAL Drupal\image\Tests\ImageDimensionsTest: test runner returned a non-zero error code (255).

I am attaching a new patch (should I have uploaded a new interdiff instead?) and have grep-ed again after my changes to confirm I have not missed anything this time (I had not double checked the last time).

Question - How should I have run changes on my local checkout only relevant to the changes I have made to catch this earlier (other that grep-ing of course)? I tried many things here - https://drupal.org/node/645286 but each resulted in various errors and I ended up not being able to run tests locally.

Thanks for all the help. Silly things I ask today should lead to better contributions in the future :-)

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented

@Sumeet.Pareek, it's OK to add the new patch as that one is tested by the testbot. But you should add also an interdiff to each new patch otherwise changes cannot be tracked by human reviewers.

Will review later the patch.

mondrake’s picture

mondrake
Location 🇮🇹
commented

5: rename-method-2109343-5.patch queued for re-testing.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented
Version: 8.0.x-dev » 8.1.x-dev
Status: Needs review » Needs work
Issue tags: +Needs reroll

Patch no longer applies. Bumping to 8.1.x, as I think interface/API changes are out for 8.0?

ErrolSilver’s picture

ErrolSilver commented
Assigned: Unassigned » ErrolSilver
Issue tags: +SprintWeekend2015
ErrolSilver’s picture

ErrolSilver commented
Status: Needs work » Needs review
StatusFileSize
new rename-method-2109343-10.patch7.8 KB

Reroll attached.

Status: Needs review » Needs work

The last submitted patch, 10: rename-method-2109343-10.patch, failed testing.

ErrolSilver’s picture

ErrolSilver commented
Assigned: ErrolSilver » Unassigned
yesct’s picture

yesct commented

@ErrolSilver
Thank you for trying to reroll this.

did #10 apply for you locally?

patch from #5 is a year old.
Might be better to try and redo the changes from there by hand since it has been so long.

balsama’s picture

balsama
he/him
Primary language English
Location boston
commented

I was mentoring Errol - we did get the patch to apply locally, but afterwards realized this issue is for the 8.1.x branch (and we were working from 8.0.x). After that, we ran out of time, but hopefully he can re-roll it again this week.

ErrolSilver’s picture

ErrolSilver commented
Assigned: Unassigned » ErrolSilver
ErrolSilver’s picture

ErrolSilver commented
Status: Needs work » Needs review
StatusFileSize
new rename-method-2109343-15.patch7.5 KB

Reroll attached

Status: Needs review » Needs work

The last submitted patch, 16: rename-method-2109343-15.patch, failed testing.

Status: Needs work » Needs review

ErrolSilver queued 16: rename-method-2109343-15.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 16: rename-method-2109343-15.patch, failed testing.

Studiographene’s picture

Studiographene commented
StatusFileSize
new Unify-effect-operation-verb-naming-in-image-style-2109343-20.patch7.81 KB

Patch re rerolled!

mandar.harkare’s picture

mandar.harkare commented
StatusFileSize
new unify-image-effect-naming-2109343-21.patch6.61 KB

Not sure what was wrong in last patch. Trying one more time.

mandar.harkare’s picture

mandar.harkare commented
Status: Needs work » Needs review

Please review.

The last submitted patch, 20: Unify-effect-operation-verb-naming-in-image-style-2109343-20.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 21: unify-image-effect-naming-2109343-21.patch, failed testing.

mandar.harkare’s picture

mandar.harkare commented
Status: Needs work » Needs review
StatusFileSize
new unify-image-effect-naming-2109343-25.patch9.14 KB

One more patch.

Status: Needs review » Needs work

The last submitted patch, 25: unify-image-effect-naming-2109343-25.patch, failed testing.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Postponed

@mandar.harkare thank you for your work, but the version of this issue is 8.1.x-dev which means tests will fail because that branch was spinned off long ago, and tests would only work against 8.0.x-dev.

In any case, I think this issue should be postponed now, since this is an API change which is unlikely to be accepted at this point of 8.0.0 development, and would need anyway a BC layer.

mandar.harkare’s picture

mandar.harkare commented

Yup @mondrake, sounds good.

Cheers!

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone’s picture

quietone commented
Status: Postponed » Active
prem suthar’s picture

prem suthar commented
StatusFileSize
new 2109343-45.patch8.31 KB

As Per the Issue tag i have re-rolled the patch again the #25 for Drupal version 11.x Dev

prem suthar’s picture

prem suthar commented
StatusFileSize
new interdiff_25_45.txt9.21 KB

Here i have added the inter-diff for the 25# To 45#.

prem suthar’s picture

prem suthar commented
Status: Active » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Should be made into MR.

Prem Suthar opened merge request !6692

prem suthar’s picture

prem suthar commented
Status: Needs work » Needs review

Move the Patch Code To MR. As per #48

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Failing a check.

smustgrave’s picture

smustgrave commented
Assigned: ErrolSilver » Unassigned

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.