Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Tagging VDC because and Twig because this is where the discussion came from. Most of which came from Views but this is all over other components as well.
Problem/Motivation
Variable clean-up for consistency.
Proposed resolution
Convert all $vars to $variables.
Wait till the twig conversions get committed to core, then convert all modules below as one patch:
Language
core/lib/Drupal/Core/Language/Language.php
Color
core/modules/color/color.module
Link
core/modules/link/link.module
Node
core/modules/node/node.views.inc
Views
core/modules/views/views.module
core/modules/views/views.theme.inc
Views UI
core/modules/views/views_ui/views_ui.module
core/modules/views/views_ui/views_ui.theme.inc
Seven
core/themes/seven/template.php
Related Issues
#1843738: [meta] Convert views module to Twig
#1757550: [Meta] Convert core theme functions to Twig templates
| Comment | File | Size | Author |
|---|---|---|---|
| #30 | 1963942-30-vars-to-variables.patch | 58.13 KB | joelpittet |
| #27 | 1963942-27-vars-to-variables.patch | 60.04 KB | joelpittet |
| #24 | 1963942-24-vars-to-variables.patch | 60.05 KB | joelpittet |
| #18 | 1963942-18-vars-to-variables.patch | 60.06 KB | dasjo |
| #18 | 1963942-18-vars-to-variables-interdiff.txt | 1.28 KB | dasjo |
Comments
Comment #1
joelpittetComment #2
joelpittetComment #3
joelpittetComment #4
joelpittetRunning test... turning to posponed after.
Comment #5
joelpittetComment #6
joelpittetConverting this to meta as per @dawehner: Each of these modules needs it's own issue.
Language
core/lib/Drupal/Core/Language/Language.php
Color
core/modules/color/color.module
Link
core/modules/link/link.module
Node
core/modules/node/node.views.inc
Views
core/modules/views/views.module
core/modules/views/views.theme.inc
Views UI
core/modules/views/views_ui/views_ui.module
core/modules/views/views_ui/views_ui.theme.inc
Seven
core/themes/seven/template.php
Also, my patch is garbage, it's including
core/vendor.Please don't use:)Comment #7
thedavidmeister CreditAttribution: thedavidmeister commentedThis is flagged as an "after commit" issue for #1757550: [Meta] Convert core theme functions to Twig templates so I'll just postpone this for now as this patch will need to be rerolled once all the Twig conversions land anyway.
Comment #8
webchickNote that if this is literally just a find/replace of vars -> variables, we don't need separate issues for all of the affected files. It's trivial to glance through the patch (even if it is quite big) and verify that the same changes are made everywhere, and we didn't accidentally change something like "varsity" into "variablesity". :)
Individual template file => Twig conversions do make sense as separate issues, because often there is markup clean-up / docs tweaks at the same time, which would be difficult to review in amongst 50,000 other changes.
Comment #9
star-szrAgreed.
Comment #10
ACF CreditAttribution: ACF commentedDone a careful find and replace and changed all the vars to variables
Comment #11
thedavidmeister CreditAttribution: thedavidmeister commented#10 - you're either going to have to re-roll that at some point or a bunch of issues #1757550: [Meta] Convert core theme functions to Twig templates will need re-rolling. I'm not sure which is preferable.
Comment #12
webchickYeah... I'd rather we postpone this one, since it's much easier to re-roll and requires far less coordination.
Comment #13
star-szrYup, this should be postponed. Thanks @ACF!
Comment #14
joelpittetI don't think this needs postponing, I separated it to get the cleanup in a bit faster. I don't mind re-rolling those mostly(if not all) views patches, as I have already un-rolled them to remove the $vars => $variables to unmuddy their patches on some.
Also, thank you @ACF for going through that:)
Comment #15
star-szrI think we must postpone this task. This cleanup will only slow down the Twig conversion and we simply cannot afford that unless our contributor base expands significantly overnight. It's also not preventing D8 from shipping, and is the very definition of 'minor'.
Comment #16
johnshortessPer @jenlampton (in the sprint just now) and @webchick in #8, removing [meta], so we can reroll one patch after #1757550.
Comment #16.0
johnshortessAdded files that need issues.
Comment #17
joelpittetRe-rolled from #10 without the vendor changes and unposponed due to twig templates being committed.
Comment #18
dasjohere's a reroll + interdiff.
theme_views_ui_reorder_displays_formseems to be gone.the patch looks pretty straight forward to me.
Comment #19
dasjoi went through the patch using
git diff --color-wordsand it seems perfectly fine.besides the vendor folder, there are no more uses of var. if tests pass again for the reroll, i'd recommend RTBCing this one :)
Comment #20
Fabianx CreditAttribution: Fabianx commentedThis looks good to me, passes tests, has been pre-RTBCed by #19. => RTBC
Comment #21
joelpittetThank you @dasjo. The interdiff is a bit funky on #18 different patch format?
Removing novice tag.
RTBC++
Comment #22
star-szr#18: 1963942-18-vars-to-variables.patch queued for re-testing.
Comment #24
joelpittetHead's moving, I caught it again.
Comment #26
joelpittet#24: 1963942-24-vars-to-variables.patch queued for re-testing.
Comment #27
joelpittetMoving target...
Comment #28
Fabianx CreditAttribution: Fabianx commentedThis looks good again:
"This looks good to me, passes tests, has been pre-RTBCed by #19. => RTBC"
Comment #29
star-szrNo longer applies.
Comment #30
joelpittetRe-rolled, if this is green it's back to RTBC;)
Comment #31
star-szrRe-reviewed with
git diff --color-words, patch looks good. Replaces all instances of $vars from core and leaves core/vendor alone.Comment #32
catchCommitted/pushed to 8.x, thanks!
Comment #33.0
(not verified) CreditAttribution: commentedUpdated proposed resolution to indicate we are updating all modules in this issue, rather than creating issues for each.