Problem/Motivation
We want to remove the process layer to simplify the theme system.
Proposed resolution
Move the building of the 'breadcrumb' render array from template_process_page() to template_preprocess_page() in preparation for removing template_process_page(). Because of #1947536: Convert drupal_get_breadcrumb() and drupal_set_breadcrumb() to a service we don't need to worry about code in the stack randomly calling drupal_set_breadcrumb(), and when the process layer is removed template_preprocess_page() the preprocess layer will be the last stage before rendering anyway.
Remaining tasks
None.
User interface changes
None.
API changes
None.
Blockers
#1939066: Convert theme_breadcrumb() to Twig
Related Issues
#1843650: Remove the process layer (hook_process and hook_process_HOOK)
#1589968: Move Title from process to preprocess, remove template_process_page()
#2004286: Defer calls to drupal_get_* functions until printed inside a template by adding a RenderWrapper class
Comment | File | Size | Author |
---|---|---|---|
#22 | 1843668-22.patch | 1.4 KB | gloob |
#22 | interdiff.txt | 545 bytes | gloob |
#20 | 1843668-20.patch | 1.41 KB | star-szr |
#15 | 1843668-15.patch | 1.44 KB | star-szr |
#15 | interdiff.txt | 1.57 KB | star-szr |
Comments
Comment #1
klonosI hope to see this make it in D8, but there's been no action for moths :/
Comment #2
bdragon CreditAttribution: bdragon commentedgrabbing @ code sprint process kill table
Comment #3
bdragon CreditAttribution: bdragon commentedfix tags
Comment #4
bdragon CreditAttribution: bdragon commentedLet's see what the tests say.
Comment #5
bdragon CreditAttribution: bdragon commented(test queue is a bit borked atm)
Comment #7
bdragon CreditAttribution: bdragon commented#4: 0001-1843668-Make-breadcrumbs-renderable.patch queued for re-testing.
Comment #9
Stefan Freudenberg CreditAttribution: Stefan Freudenberg commentedThis depends on https://drupal.org/node/2004286 to be committed.
Comment #10
Stefan Freudenberg CreditAttribution: Stefan Freudenberg commentedSetting status to postponed.
Comment #10.0
Stefan Freudenberg CreditAttribution: Stefan Freudenberg commentedupdate
Comment #11
jenlamptonActually, closing as a dupe of https://drupal.org/node/1939066
Comment #11.0
jenlamptonadded defered wrapper for render on print
Comment #12
jenlamptonOkay, not closed/dupe. Just blocked by #1939066: Convert theme_breadcrumb() to Twig
This is going to need a reroll once that gets in (to remove the conversion).
Changing to needs work.
Comment #13
joelpittetI know this won't work but thought I'd post for someone to work with from. There are a few errors that the testbot will catch.
This is just the remaining pieces from #4 without the twig conversion from #1939066: Convert theme_breadcrumb() to Twig
Comment #15
star-szrTrying this simple patch that just moves the breadcrumb variable from process to preprocess. Seems to behave locally and passes the tests that failed above - didn't check all the tests with exceptions, those just look like invalid render array fallout.
Interdiff from #13.
(I think this is got a lot simpler now that #1947536: Convert drupal_get_breadcrumb() and drupal_set_breadcrumb() to a service is in.)
Comment #15.0
star-szrblockers
Comment #16
jenlamptonI thought we wouldn't need to do anything special to move these up to process now, but I kept thinking myself around in circles.
This looks great (testing patch locally) and breadcrumbs are working for me. Yay!
Comment #16.0
jenlamptonAdd link to process meta
Comment #16.1
star-szrUpdate issue summary to reflect latest patch
Comment #17
star-szrRe-titling.
Comment #18
YesCT CreditAttribution: YesCT commentedThis issue was RTBC and passing tests on July 1, the beginning of API freeze.
Comment #19
alexpottNeeds a reroll...
Comment #20
star-szrRerolled.
Comment #21
penyaskitoUse Drupal::request() instead of Drupal::service('request').
Otherwise, looks good to me.
Comment #22
gloob CreditAttribution: gloob commentedChanging Drupal::service('request') by Drupal::request()
Testing contributing workflow in Drupal hope you don't mind Cottser ;-)
Comment #23
penyaskitoBack to RTBC after the re-roll.
Thanks :)
Comment #24
star-szrNot at all, thanks @gloob and @penyaskito :D
Comment #25
jenlamptonnm
Comment #26
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks.
Comment #27
effulgentsia CreditAttribution: effulgentsia commentedI think it's fine that this was committed. However, the following from the issue summary is incorrect:
Every module's and theme's hook_preprocess_page() implementation runs after template_preprocess_page().
Delaying the breadcrumb build until after all module and theme preprocess functions were done was added in #907690-24: Breadcrumbs don't work for dynamic paths & local tasks #2 because of performance issues that were around at that time. I don't know yet whether the concerns from then are still valid or not, so I left a comment on that issue linking to this one.
Comment #28
star-szrThanks @effulgentsia! I realize after reviewing that it wasn't quite right. What I meant to say was that the preprocess layer in general would be the last stage before rendering, not specifically template_preprocess_page().
Comment #28.0
star-szrBetter wording
Comment #29.0
(not verified) CreditAttribution: commentedCorrection regarding template_preprocess_page()