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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

effulgentsia created an issue. See original summary.

xjm’s picture

This should address this feedback from Wim:

+++ b/core/modules/outside_in/outside_in.module
@@ -0,0 +1,140 @@
+function outside_in_page_top(array &$page_top) {
+  // Opens a div for consistent wrapping to {{ page }} render in all themes.
+  $page_top['outside_in_tray_open'] = [
+    '#markup' => '<div id=“main-canvas-wrapper”><div id=“main-canvas”>',
+    '#weight' => 1000,
+  ];
+}
+
+/**
+ * Implements hook_page_bottom().
+ */
+function outside_in_page_bottom(array &$page_bottom) {
+  // Closes a div for consistent wrapping to {{ page }} render in all themes.
+  $page_bottom['outside_in_tray_close'] = [
+    '#markup' => '</div></div>',
+    '#weight' => -1000,
+  ];
+}

Hah :)
Not convinced this is the appropriate long-term solution, but absolutely fine for experimental.

tedbow’s picture

Component: quickedit.module » outside_in.module
martin107’s picture

Status: Active » Needs review
FileSize
2.15 KB

I 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!

tim.plunkett’s picture

  1. +++ b/core/modules/outside_in/outside_in.module
    @@ -52,24 +52,25 @@ function outside_in_block_view_alter(array &$build) {
    +  if(isset($type['page'])){
    

    missing spaces around the ()

  2. +++ b/core/modules/outside_in/outside_in.module
    @@ -52,24 +52,25 @@ function outside_in_block_view_alter(array &$build) {
    +    if(!isset($type['page']['#theme_wrappers'])){
    +      $type['page']['#theme_wrappers'] = [];
    +    }
    +    $type['page']['#theme_wrappers'] = array_merge($type['page']['#theme_wrappers'], ['outside_in_page_wrapper' => ['#weight' => -1000]]);
    

    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];

Status: Needs review » Needs work

The last submitted patch, 4: wrapper-2784463-4.patch, failed testing.

tim.plunkett’s picture

martin107’s picture

Status: Needs work » Needs review
FileSize
928 bytes
2.18 KB

Thanks 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

Status: Needs review » Needs work

The last submitted patch, 8: wrapper-2784463-8.patch, failed testing.

naveenvalecha’s picture

naveenvalecha’s picture

Title: Convert outside_in_page_(top|bottom)() to a #theme_wrappers » [PP-1] Convert outside_in_page_(top|bottom)() to a #theme_wrappers
naveenvalecha’s picture

Status: Needs review » Postponed
naveenvalecha’s picture

Title: [PP-1] Convert outside_in_page_(top|bottom)() to a #theme_wrappers » Convert outside_in_page_(top|bottom)() to a #theme_wrappers
Status: Postponed » Needs review
naveenvalecha’s picture

martin107’s picture

Thanks for turning the patch green

naveenvalecha++

tim.plunkett’s picture

+++ b/core/modules/outside_in/outside_in.module
@@ -52,24 +52,26 @@ function outside_in_block_view_alter(array &$build) {
+    if (!isset($type['page']['#theme_wrappers'])) {
+      $type['page']['#theme_wrappers'] = [];
+    }
+    $wrapper = ['outside_in_page_wrapper' => ['#weight' => -1000]];
+    $type['page']['#theme_wrappers'] = array_merge($type['page']['#theme_wrappers'], $wrapper);

I 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.

martin107’s picture

Thanks for getting to the bottom of that .... the next patch is more readable.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Thanks! I think this is good to go.

andypost’s picture

how that possible to have no page element?

+++ b/core/modules/outside_in/outside_in.module
@@ -52,24 +52,22 @@  function outside_in_element_info_alter(&$type) {
+  if (isset($type['page'])) {
+    $type['page']['#theme_wrappers']['outside_in_page_wrapper'] = ['#weight' => -1000];
+  }

I think isset() should be removed because we should always have this element and once "page" removed we should get errors

tim.plunkett’s picture

Just 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.

catch’s picture

Status: Reviewed & tested by the community » Needs work

Yes that's fair, it's going to fail much worse than a notice if that's missing.

tim.plunkett’s picture

I really couldn't disagree more.

tim.plunkett’s picture

If we were to unit test this function, we would NOT want it to add to $type unless $type['page'] exists. It's that simple.

  • catch committed 4beaddb on 8.3.x
    Issue #2784463 by martin107, naveenvalecha, tim.plunkett: Convert...

  • catch committed db9b5ef on 8.2.x
    Issue #2784463 by martin107, naveenvalecha, tim.plunkett: Convert...
catch’s picture

Status: Needs work » Fixed

Yes 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!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

tedbow’s picture

Component: outside_in.module » settings_tray.module

Changing to new settings_tray.module component. @drpal thanks for script help! :)