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.
Problem/Motivation
Individual render elements should not render as malformed HTML (e.g., unclosed DIV tags), which is what the current implementation of outside_in_page_top() is.
Proposed resolution
Implement hook_element_info_alter() instead and add a #theme_wrappers to the 'page' type.
Remaining tasks
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#17 | 2784463-17.patch | 2 KB | martin107 |
Comments
Comment #2
xjmThis should address this feedback from Wim:
Comment #3
tedbowComment #4
martin107 CreditAttribution: martin107 commentedI have borrowed heavily from container.html.twig
I have manually tested this and can't make it fall over. Visually the html seems unaffected.
To criticise my own work the name "outside-in-page-wrapper.html.twig" is one of those
"very-wordy-things-with-too-many-words-in-it"
suggestions welcome!
Comment #5
tim.plunkettmissing spaces around the ()
This may be because I'm reading this after a long day, but why can't this be:
$type['page']['#theme_wrappers']['outside_in_page_wrapper'] = ['#weight' => -1000];
Comment #7
tim.plunkettFailure is due to #2785913: StableTemplateOverrideTest does not respect experimental modules
Comment #8
martin107 CreditAttribution: martin107 as a volunteer commentedThanks for pointing out that in #7 --- that has saved me lots of time!
regarding #5
1) Fixed .. thanks
2) I would be happy to simplify .. MAYBE it is a detail we can simplify.
I saw that care had been taken to specify a weight in the the original version.
which made me think that we may not be the only code thread that wants to wrap the page.
Plus when I looked in language_element_info_alter() [ language.module]
I could see other examples where care is taken not to trash the existing theme_wrappers.
PS refactored outside_in_element_info_alter to shorten a line that was over 120 characters long
Comment #10
naveenvalechaAttached is the combined patch of #2784463-8: Convert outside_in_page_(top|bottom)() to a #theme_wrappers and #2785913-3: StableTemplateOverrideTest does not respect experimental modules drupal ci would be happy now.
Also interdiff is a change in #2784463-8: Convert outside_in_page_(top|bottom)() to a #theme_wrappers
Comment #11
naveenvalechaPostponed it on #2785913: StableTemplateOverrideTest does not respect experimental modules until that went in.
Comment #12
naveenvalechaComment #13
naveenvalechaUnpostponing this one as this got committed #2785913: StableTemplateOverrideTest does not respect experimental modules
Comment #14
naveenvalechaReuploaded the patch after #2785913: StableTemplateOverrideTest does not respect experimental modules
Comment #15
martin107 CreditAttribution: martin107 as a volunteer commentedThanks for turning the patch green
naveenvalecha++
Comment #16
tim.plunkettI looked it up, and the origin of this array_merge pattern was actually for a set of #process callbacks, where the order mattered. Let's go with the more straightforward version I proposed in #5.
Comment #17
martin107 CreditAttribution: martin107 as a volunteer commentedThanks for getting to the bottom of that .... the next patch is more readable.
Comment #18
tim.plunkettThanks! I think this is good to go.
Comment #19
andyposthow that possible to have no page element?
I think isset() should be removed because we should always have this element and once "page" removed we should get errors
Comment #20
tim.plunkettJust because Drupal currently requires the system.module, which obviously provides a page element, doesn't mean we should be sloppy or less defensive in our code.
Comment #21
catchYes that's fair, it's going to fail much worse than a notice if that's missing.
Comment #22
tim.plunkettI really couldn't disagree more.
Comment #23
tim.plunkettIf we were to unit test this function, we would NOT want it to add to $type unless $type['page'] exists. It's that simple.
Comment #26
catchYes good point. If it was contrib altering something else I'd want the isset(), just because we never expect this to go away doesn't mean the same rules don't apply. Too much time arguing against overly defensive code elsewhere.
Committed/pushed to 8.3.x and cherry-picked to 8.2.x. Thanks!
Comment #28
tedbowChanging to new settings_tray.module component. @drpal thanks for script help! :)