Split off from #2416831: Add an active_theme twig function.

Problem/Motivation

The TwigExtension class has a setGenerators method which probably should have always been called setUrlGenerator because according to git log that's all it ever injected.

Proposed resolution

Change it, or talk about a better way and do that instead.

Remaining tasks

Patch.
Patch review.

User interface changes

n/a

API changes

TwigExtension setGenerators will change to setUrlGenerator.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task, because its not a bug, it cleans things up
Issue priority normal, because the positive impact is not dramatically.
Prioritized changes Correcting misspelled or wholly inaccurate method names is prioritized.
Disruption Not disruptive, its a internal method, and we are providing a BC-layer in case anyone else uses it.
Files: 
CommentFileSizeAuthor
#10 change_setgenerators_to-2496801-10.patch2.19 KBcilefen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,427 pass(es). View
#10 interdiff.txt1.62 KBcilefen
#1 change_setgenerators_to-2496801-1.patch1.79 KBCottser
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,274 pass(es). View

Comments

Cottser’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
1.79 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,274 pass(es). View
Cottser’s picture

Cottser’s picture

tstoeckler’s picture

Once the beta evaluation is written, this is RTBC. This has confused me many times and I don't see any disruption caused by this change.

dawehner’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs beta evaluation

I like the better name!

cilefen’s picture

+1

If anyone is concerned enough about changing this method name, we could leave the existing method, marked deprecated, for BC.

Cottser’s picture

The only disruption might be if someone is extending core's TwigExtension, but 99.99% of the time they shouldn't be doing that anyway. https://www.drupal.org/node/1831138

tstoeckler’s picture

Right and even then it's not likely (though certainly possible) that something will break.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Let's do this with a deprecation rather than removal - therefore nothing that extends TwigExtension will break. The lack of disruption is not a reason to commit something within beta. See the flowchart on https://www.drupal.org/core/beta-changes.

Renaming a misleading method name needs committer approval and should be done in a non-breaking way. I talked to @xjm and we approve of adding a new method setUrlGenerator() and the deprecation of setGenerators() for Drupal 9.

Also the beta evaluation needs fixing as the prioritized changes has not been completed.

cilefen’s picture

Status: Needs work » Needs review
FileSize
1.62 KB
2.19 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,427 pass(es). View
cilefen’s picture

Issue summary: View changes
Cottser’s picture

Interdiff is misleading but patch looks good. Thanks @cilefen and thank you for setting us straight @alexpott!

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Alright, sheep it

  • alexpott committed f08e28a on 8.0.x
    Issue #2496801 by cilefen, Cottser: Change setGenerators to...
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed f08e28a and pushed to 8.0.x. Thanks!

Thanks for adding the deprecation/bc layer and adding the beta evaluation to the issue summary.

Status: Fixed » Closed (fixed)

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