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.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

star-szr’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
1.79 KB
star-szr’s picture

star-szr’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.

star-szr’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
cilefen’s picture

Issue summary: View changes
star-szr’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.