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.
Task
Convert PHPTemplate templates to Twig templates.
Remaining
- Patch needs review
- Manual testing
Template path | Conversion status |
---|---|
core/modules/system/templates/system-plugin-ui-form.tpl.php | converted |
Related
#1987410: [meta] system.module - Convert theme_ functions to Twig
#1757550: [Meta] Convert core theme functions to Twig templates
#1938930: Convert theme_system_modules_details() to #type table
#1987426: Convert maintenance-page.tpl.php to Twig
Comment | File | Size | Author |
---|---|---|---|
#62 | 1898454-62-twig-system-tpl.patch | 3.04 KB | joelpittet |
#62 | interdiff.txt | 1.97 KB | joelpittet |
#59 | 1898454-56.patch | 2.65 KB | Sean Charles |
#56 | 1898454-56.patch | 2.65 KB | mr.baileys |
#53 | drupal-1898454-53-interdiff.txt | 1.99 KB | Hydra |
Comments
Comment #1
c4rl CreditAttribution: c4rl commentedTagging
Comment #2
trrroy CreditAttribution: trrroy commentedComment #3
c4rl CreditAttribution: c4rl commentedComment #4
c4rl CreditAttribution: c4rl commentedRelated #1905584: Move base theme system templates into /core/templates
Comment #5
c4rl CreditAttribution: c4rl commentedRe #4, we'll leave all the core/include conversions to their respective issues.
Comment #6
c4rl CreditAttribution: c4rl commentedI will plan to deprecate
theme_system_modules_incompatible()
in the conversion here. This theme callback only provides one wrapperdiv
with a single HTML class. The utility is trivial.It is called by a parent function
system_modules()
that paradoxically provides hard-coded markup for this page. Arguablysystem_modules()
should be refactored so that markup is provided in an overridable way.Comment #7
star-szrAny news on this patch/issue? Even a rough work-in-progress patch would be a great start, or if you won't have time to look at this we can get some other folks working on it. Thanks!
Comment #8
c4rl CreditAttribution: c4rl commentedI'll try to have a patch in the next 24 hours.
Comment #9
c4rl CreditAttribution: c4rl commentedHere's a rough first-pass. There's a lot to consolidate in this module, some theme functions are duplicates (see #1842232: Consolidate use of admin-page.html.twig and system-admin-index.html.twig) and things like admin_block and admin_block_content could likely be combined.
I'm also guessing #1876712: [meta] Convert all tables in core to new #type 'table' will have impact here as well as what I'm proposing in #202593-19: 'Hide descriptions' link on admin/config affects other pages.
Comment #11
c4rl CreditAttribution: c4rl commentedAccidentally removed the datetime template, which should actually be resolved with #1905584: Move base theme system templates into /core/templates
Comment #13
star-szrThanks for posting your patch. 4 fails, that's nothing! :D
I'd recommend removing the change to datetime.html.twig - I don't think we need @todos for every template affected by #1905584: Move base theme system templates into /core/templates, and this will be the third conversion issue touching datetime.html.twig and causing conflicts – the other issue is #1898480: [meta] form.inc - Convert theme_ functions to Twig. I'm hoping we can get it down to just #1939080: Convert theme_datetime() to Twig updating that template. Feel free to roll that @todo into the patch there if you like.
Comment #14
c4rl CreditAttribution: c4rl commentedThis should do it. drupal_render()/drupal_render_children() can be irritating, hoping that #1876712: [meta] Convert all tables in core to new #type 'table' solves these issues.
Comment #15
c4rl CreditAttribution: c4rl commentedComment #17
star-szrI think we can remove any theme function conversions from this patch that are converted to #type table via #1938930: Convert theme_system_modules_details() to #type table:
theme_system_date_format_localize_form()
theme_system_modules_uninstall()
Comment #18
star-szr#14: drupal-1898454-14.patch queued for re-testing.
Comment #19
c4rl CreditAttribution: c4rl commentedGood idea. Here's the patch without those theme callbacks converted.
Comment #20
c4rl CreditAttribution: c4rl commentedCottser just reminded me in IRC that the datetime template shouldn't have any changes as not to conflict with #1939080: Convert theme_datetime() to Twig. Reverting these then.
Comment #22
star-szr#20: drupal-1898454-20.patch queued for re-testing.
Comment #23
star-szrHave I mentioned that green is my favourite colour? Nice work c4rl :)
Comment #24
tstoecklerFound a bunch of nitpicks. Looks like very nice clean-up. Leaving at needs review, as this can be fixed post-commit, if needed.
I'd like to meet the themer that goes strolling around line 2000 of system.admin.inc for template/theming examples. :-)
Seriously, though, I think this should be removed.
There needs to be a blank (" *") line between these two. Also "It" should probably be "The template" (or "This template"?).
There should be a blank line here as well. Also no colon after the @todo and start *R*emove with a capital letter.
More importantly, I think you mean remove this *function*, not this file, right?
Trailing whitespace.
This should be preceded by a blank (" *") line. Also, the second line should be indented by 2 chars.
I was about to say that this should be removed as well, but then I realized that people might search for this on api.drupal.org and for them this would be pretty nice. So maybe we should keep this. (This applies to the above function as well, of course.)
Comment #24.0
star-szrAdd conversion summary table
Comment #24.1
star-szrUpdate remaining
Comment #25
star-szrTagging.
Comment #26
c4rl CreditAttribution: c4rl commentedCleaned-up documetation per #24 and #1913208: [policy] Standardize template preprocess function documentation.
Comment #27
star-szr@steveoliver said he'd review this one :)
Comment #28
steveoliver CreditAttribution: steveoliver commentedUpdate: will be reviewing this Thursday, incase anyone else wants to review in the meantime.
Comment #29
jenlamptonchanging back to needs work since this patch won't apply anymore.
Comment #30
duellj CreditAttribution: duellj commented#26: drupal-1898454-26.patch queued for re-testing.
Comment #32
c4rl CreditAttribution: c4rl commentedI'll re-roll. Also see #1876712-35: [meta] Convert all tables in core to new #type 'table'.
I kind of also want to kill template_preprocess_confirm_form and template_preprocess_system_settings_form since they are just examples.
Comment #33
c4rl CreditAttribution: c4rl commentedI've re-rolled with the following changes:
* I've removed confirm_form and system_settings_form since they were just empty examples and aren't really helpful.
* I've left out the conversion of theme_system_modules_details since it seems that should be handled by #1938930: Convert theme_system_modules_details() to #type table instead.
Interdiff is with respect to #26.
Comment #34
c4rl CreditAttribution: c4rl commentedComment #34.0
c4rl CreditAttribution: c4rl commentedUpdate remaining
Comment #35
Fabianx CreditAttribution: Fabianx commented#33: drupal-1898454-33.patch queued for re-testing.
Comment #37
star-szrNeeds a reroll after #1964044: Change module list in install/uninstall confirm form to render array got committed.
Comment #38
c4rl CreditAttribution: c4rl commentedPer #1757550-44: [Meta] Convert core theme functions to Twig templates, retitling to indicate this issue applies to templates rather than theme_ functions. See #1987410: [meta] system.module - Convert theme_ functions to Twig for theme_ function conversion.
Comment #39
c4rl CreditAttribution: c4rl commentedMissed tag?
Comment #39.0
c4rl CreditAttribution: c4rl commentedRemove sandbox link
Comment #39.1
c4rl CreditAttribution: c4rl commentedList of templates and related issu
Comment #40
gnugetComment #41
gnugetBefore to start with #38
I did a reroll because of this #1964044: Change module list in install/uninstall confirm form to render array and this #1696224: Remove system_settings_form()
(I'm going to put this issue as needs review for the testbot check if anything is broken, after that i will provide a new patch only with the tpl.php files conversion)
Comment #42
gnugetA patch with only the PHPTemplate conversion.
Comment #43
steveoliver CreditAttribution: steveoliver commentedWow, this is the only template in system? Right on.
Small nitpicks:
Lose the dollar signs.
Indent div and use whitespace dashes, like this:
Comment #44
c4rl CreditAttribution: c4rl commented@steveoliver's nits from #43
Comment #45
star-szrTagging for profiling.
Comment #46
star-szrCan we just say 'form elements' instead of 'form array elements' here?
Going to do some testing and profiling here.
Comment #47
star-szrMarkup looks great. Happy to RTBC this after the minor docs tweak in #46 is done.
Profiling
I added a views block displaying a node to the sidebar to isolate the profiling to just this conversion - this way Twig would be loaded for both scenarios.
Stark theme, APC class loader enabled - tested the page at admin/structure/block/list/block_plugin_ui%3Astark/add.
With Twig already loaded, this conversion is on par or even slightly faster than the PHPTemplate version.
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51901ff69a8a6&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51901ff69a8a6&...
Comment #48
c4rl CreditAttribution: c4rl commentedTweaks from #46 (Eventually these columns should just die and be replaced with a layout, IMHO -- separate issue). RTBC?
Comment #49
star-szrRTBC (when it comes back green), thanks @c4rl!
The only other thing is we will need to rework template_preprocess_system_plugin_ui_form() after #1920886: drupal_render() should check if it's rendering a 'render element' and if so call drupal_render_children() (inline) instead is resolved - only the drupal_add_css() line would be needed.
Comment #50
star-szr…and the template would look like this, so template docs would need tweaking as well:
Comment #51
alexpott#1920886: drupal_render() should check if it's rendering a 'render element' and if so call drupal_render_children() (inline) instead has landed this needs work...
Comment #52
LewisNymanI got this error after applying the patch, is there something else I need to do?
PHP Fatal error: Class '__TwigTemplate_b34682e870a2789958fdc767a660c4e1' not found in /Users/Lewis/Sites/DrupalCoreIssues/drupal/core/lib/Drupal/Core/Template/TwigEnvironment.php on line 107
Comment #53
Hydra CreditAttribution: Hydra commented@LewisNyman: The patch in #1920886: drupal_render() should check if it's rendering a 'render element' and if so call drupal_render_children() (inline) instead was not commited/pushed properly, this may cause your problems?
Anyhow, this will be done hopefully soon, so here the updated patch. Keep in mind, it's still depending on the patch in #1920886: drupal_render() should check if it's rendering a 'render element' and if so call drupal_render_children() (inline) instead.
Comment #55
mr.baileysAssigning to myself for a reroll.
Comment #56
mr.baileysRe-rolled so the patch applies.
Comment #57
mr.baileysComment #59
Sean Charles CreditAttribution: Sean Charles commentedRe-rolled (form variable reference)
Comment #60
Sean Charles CreditAttribution: Sean Charles commentedComment #61
Hydra CreditAttribution: Hydra commented#56: 1898454-56.patch queued for re-testing.
Comment #62
joelpittetDoc updates and did manual testing between .
And there was only minor whitespace diffs.
Comment #63
Hydra CreditAttribution: Hydra commented+1 for RTBC
Comment #64
star-szrLet's re-profile this one now that we have the drupal_render_children() update.
Comment #65
c4rl CreditAttribution: c4rl commentedComment #66
star-szr@c4rl - this is one of the only ones left so I'm going to unassign so anyone can take this on, since I'm not sure if you got results yet. Feel free to still go through this yourself, more profiling data is not a bad thing :)
Comment #67
jerdavisComment #68
jerdavisProfiled and uploaded.
I had some odd results on this one, most half runs were -0.4% to 0.3%, a couple were in the 1.4%-2.2% range. I'm uploading the one that seemed the cleanest.
Scenario:
* Full node block placed in content region
* 50 nodes are generated on site
* Applied patch
* Gave Anonymous user Administer blocks
* Pointed test to admin/structure/block/list/block_plugin_ui%3Astark/add
Results:
Comment #69
Fabianx CreditAttribution: Fabianx commentedI verified profiling results, +1 for RTBC.
Comment #70
alexpott+1. Ready for #1987510: [meta] Convert all core *.tpl.php templates to Twig as singular patch
Comment #71
alexpottCommitted 67b166c and pushed to 8.x. Thanks!
Comment #72.0
(not verified) CreditAttribution: commentedAdd maintenance page conversion issue