Problem/Motivation

RenderWrapper was meant to be a stop-gap to remove the process layer. It served it's purpose.

The idea is that once assetic was in there would be no need for this. #1762204: Introduce Assetic compatibility layer for core's internal handling of assets and being removed here :( #1784774: Remove Assetic component from core Well assetic is taking it's time, and there is no need for it now!

Though while working on auto-escape on by default in twig @chx and I found that RenderWrapper caused all kinds of PITA issues and exception workarounds. #1825952: Turn on twig autoescape by default
A few things that used to need RenderWrapper and there are just a few stragglers, namely drupal_get_js() and drupal_get_css() and a closure for meta and links.

Also I hate this class name with a passion, I came up with it and it's haunting me! Hated it also when it was created too. So please, please remove this

poorly named magic wonder box of shame

Proposed resolution

Add helpers to HTMLFragment and call those helpers from the template.
Remove the class.

Remaining tasks

  • Write a patch
  • Cleanup coding standards, docs.
  • Write change record.

User interface changes

n/a

API changes

Removal of silly wonder box. (aka, "I wonder why they called it that, I don't know what the **** it does...").

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joelpittet’s picture

sun’s picture

Very unhappy with these changes:

+++ b/core/modules/book/templates/book-export-html.html.twig
@@ -22,7 +22,8 @@
-    {{ head }}
+    {{ html.metaElements|join('\n') }}
+    {{ html.linkElements|join('\n') }}

+++ b/core/modules/system/templates/html.html.twig
@@ -29,17 +29,19 @@
-    {{ head }}
+    {{ html.metaElements|join('\n') }}
+    {{ html.linkElements|join('\n') }}
...
-    {{ page_bottom }}
+    {{ html.bodyBottom }}
+    {{ html.scripts('footer') }}

→ Perhaps it does fulfill a purpose today, but it's different to the originally intended one?

joelpittet’s picture

@sun Are you unhappy with the names? The joins? or the approach?

Because I can fix the names, and I can join them before the template...

markhalliwell’s picture

I know this is probably a little off topic/out of scope, but ultimately I think it should resemble something like this in the end.

<!DOCTYPE html>
<html{{ html.attributes }}>
  <head>
    {{ page.head }}
    <title>{{ page.title }}</title>
    {{ page.styles }}
    {{ page.scripts|without('footer') }}
   </head>
   <body{{ page.attributes }}>
     <a href="#main-content" class="visually-hidden focusable skip-link">
       {{ 'Skip to main content'|t }}
     </a>
    {{ page.top }}
    {{ page.content }}
    {{ page.bottom }}
    {# print remaining scripts #}
    {{ page.scripts }}
   </body>
 </html>

I am curious though how you can "join them before the template"? We can't do it in preprocess because themes can preprocess after core/contrib. Process is gone (which introduced RenderWrapper).

joelpittet’s picture

Ok well it's likely an aesthetic aspect of it... so here's a 'prettier' version. I left {{ page.bodyTop }} and {{ page.bodyBottom }}. because those were named in HtmlPage already and we are just needlessly wrapping them in a renderable array of type #markup, though that could easily be changed to HtmlPage::getTop() and HtmlPage::getBottom().

@Mark Carver seems you are on the same aesthetic pov from that comment as @sun which I expected a few people would have. And for {{ page.attributes }} there are helper methods on HtmlPage class for {{ page.htmlAttributes }} and {{ page.bodyAttributes }} which "just work". Though for now I can just leave those as is and we can open up a follow up. Also I can do the same for {{ page_top }} ... {{ page_bottom }} but just want to show them in this patch for example purposes.

And side note:{{ scripts|without('footer') }} looks like "everything looks like a nail" syndrome, though I appreciate the thought:) You'd have to rework the structure to have the results somehow nested in their 'scope' to have that work and feels a bit more awkward in that configuration.

joelpittet’s picture

FileSize
8.84 KB

Tried git format-patch above and it didn't seem to work as I'd hopped so here my normal workflow... ignore #5 patch.

joelpittet’s picture

And here is just removing the page_top and page_bottom changes because they aren't needed here.

Status: Needs review » Needs work

The last submitted patch, 7: 2272279-die-RenderWrapper-7.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
675 bytes
8.62 KB

Woops, changed one too many variables on book export.

The last submitted patch, 5: 2272279-Die-RenderWrapper-5.patch, failed testing.

joelpittet’s picture

The last submitted patch, 6: 2272279-die-RenderWrapper-6.patch, failed testing.

Crell’s picture

Doing something like this is absolutely what my original intent for HtmlPage was eventually, so +1 on the general idea.

That said, I have 2 pushbacks:

1) I disagree with sun 100%. Pushing the string concatenation into HtmlFragment is polluting the domain object, whose job is to represent an HTML snippet/page, not to format an HTML snippet/page. There's really nothing wrong at all with the join() calls in the template. Twig is fully capable of doing that, it's totally legit, and it is display-level logic. Display level logic belongs at the display level, not baked into a domain object just to save a few keystrokes in the template.

Debatably we could have a getHeadElements() utility method on HtmlPage that wraps calls to the other getters, but frankly I think that's entirely unnecessary. (It also means the template cannot control the order in which the various elements appear, which IMO belongs there and not in HtmlPage.)

2) The styles/JS methods are quiet clearly temporary hacks. I would feel a bit more comfortable doing that as part of putting for-realsies [add|get]Script()/[add|get]Style() methods to HtmlFragmentInterface. Then the HtmlPage implementation of those (NOT the HtmlFragment class's version, that's entirely the wrong place) can also throw in the BC function for as long as it's needed. That may or may not be implicit in #2256365: Factor render->fragment code out to a service; we should discuss what the optimal order of actions is here, given that we want digestable patches but patch commits are often annoyingly slow. :-(

Also, this is going to conflict a bit with #2256373: Factor HtmlFragment out to an interface which is RTBC, so let's let that land first.

RainbowArray’s picture

It may be entirely legit to put those join \n statements into the template, but the vast majority of people working on a theme's templates are going to have no clue as to why that's there. If that logic is in the template, it's the exact same problem as putting it into preprocess. A theme may or may not end up doing things that way. The html template is customized very frequently, so it's definitely possible that somebody could take a look at those join statements, say "what the heck are those," delete the odd-looking code, and move on. If those join statements have a significant impact on whether or not something works correctly, that could cause a problem.

Just doesn't feel like the sort of thing that belongs in a template.

Crell’s picture

If the theme may or may not end up doing it that way, that's all the more reason the theme should be responsible for that final step rather than HtmlPage. (Although why someone would be customizing html.html.twig on a regular basis I don't know.)

Also, if someone goes "I don't know what join means, I'll just delete it", first of all they're being very naive. Second of all, their page will break with a PHP error on the very next load so they'll know to go put them back. :-)

RainbowArray’s picture

From what you've described it doesn't sound like there is a meaningful reason to have this in the template, in that it's not something that should be customized. If it is something that is always needed no matter what, and any tweaks might break the site, then having it in templates seems not ideal.

Is there any other way to accomplish this join besides a template?

joelpittet’s picture

Thanks for you the feedback @Crell and @mdrummond.

I'm siding with the nice looking one for the theme, aka {{ page.head }} because the way I wrote the code it doesn't prevent them from doing |join() way because those methods on the HtmlPage object still are public and accessible to the template. So we both win on that one and it's moot.

@Crell to you're point I have the methods added to the wrong object. I'll try to fix that in the next patch. That's why I asked and hope we can add these little helpers to the HtmlPage and leave the HtmlFragement alone. (i'll try atleast).

Also, I'll try to be cognizant of those other issues but this issue is for me an autoescape blocker(which is a beta blocker) so the least change but still remove RenderWrapper is the goal here and these helpers on the HtmlPage would really help out a lot! Maybe we can strike a deal here? If we can't come to a consensus here I'll try to find you to discuss in person next weekend in Austin.

@mdrummond you can use |join() or you can loop through the array and print the items, those are the two options that I know of, both not as 'clean looking' but very important that themers know their options and we don't limit them from getting at the data to theme. Sure I can provide a string but as @Crell mentioned we shouldn't limit the themers by providing already baked markup... I've provided both options avaiable by sending the HtmlPage object directly to the template and all it's public methods are callable.

Personally I'm glad we can do the join and that those public methods exist on that object and that I can make them available to the template, it's a huge win for me personally. I don't want to prevent or break any features and this would both provide a nicety to the template but also not limit the possibilities of manipulating in preprocess or in the template at getting the values or manipulating the values.

joelpittet’s picture

Also re #15 I was going to say that too... thanks. They'll learn fast:)

I think we can compromise here and all win, so hopefully we can proceed?

joelpittet’s picture

Moved methods from HtmlFragment to HtmlPage. HtmlFragment just provides methods getMetaElements and getLinkElements that getHead() uses.

Twig can access any getWhatEver public method automatically when printing, that's why this change gives a lot of power to the template.

sun’s picture

That looks much better now, thanks! I'd consider this RTBC in case testbot + @Crell agrees.

RainbowArray’s picture

Revised templates look a lot better. Thanks Joel.

Crell’s picture

  1. +++ b/core/lib/Drupal/Core/Page/HtmlPage.php
    @@ -78,6 +78,40 @@ public function getHtmlAttributes() {
    +  public function getHead() {
    +    return implode("\n", $this->getMetaElements()) . implode("\n", $this->getLinkElements());
    +  }
    

    I still disagree with this. It pollutes the domain object with rendering information and removes flexibility from themers. That's a lose-lose.

  2. +++ b/core/lib/Drupal/Core/Page/HtmlPage.php
    @@ -78,6 +78,40 @@ public function getHtmlAttributes() {
    +  public function getScripts($scope = 'header') {
    +    return drupal_get_js($scope);
    +  }
    +
    +  /**
    +   * Returns a themed representation of all stylesheets to attach to the page.
    +   *
    +   * @return
    +   *   A string of XHTML CSS tags.
    +   *
    +   * @see drupal_get_css()
    +   */
    +  public function getStyles() {
    +    return drupal_get_css();
    +  }
    

    Another problem. These methods as written would be returning strings. getMetaElements() and getLinkElements() are returning an array of objects. (Those objects implement __toString() as there's only one logical way to stringify them.)

    It's inconsistent to make one objects and the other strings. When we add proper add*() methods for those we are going to have to define a StyleElement and ScriptElement class to parallel the others. In fact, for the various aggregation/compression work going on we will NEED that data in a structured format so that we can pull it out, manipulate it to refer to a generated/compressed file instead, then put that information back on the object. (Or potentially return a new object instead, which would likely be easier.) String-ified functions here are not going to work.

    Since we still need to get in the interface issue first anyway, let's go ahead and turn this issue into the one that defines those classes and makes use of them. I was hoping that we would do that in the Assetic issue, but... yeah.

joelpittet’s picture

@Crell re #22

  1. It provides exactly what they get now AND because I'm passing the object to the template they have access to those magic getters too if they want that flexibility to use the arrays directly, so it's a win-win in my books.
  2. I'd love to rewrite drupal_get_css() and drupal_get_js(), but I don't have time to do that the correct way atm... so this is the quick and dirty get the job done way. Assetic or something else can deal with that problem. We can add a "getStyleElements()" and "getScriptElements()" and return the arrays if that helps sell this patch? I'm trying to avoid changing the API of known drupal code. Those functions are doing what they've always done, return a string of elements to the head and footer. RenderWrapper was just to get rid of preprocess and before Assetic or something better comes along to objectify the asset building.

What is the MVC (minimum viable code) to remove RenderWrapper from core?

joelpittet’s picture

@Crell Sorry, I'm quite sure we are all busy this week more than most, that message sounded short tempered. I'll revisit this likely on the weekend at an extended sprint or something and see what we can do.

Maybe we can discuss some options in person if you're around for them too?

Crell’s picture

I get into Austin Sunday evening. I'll be at the sprint Monday morning and would be happy to discuss the roadmap for this part of the code then. (It sounds like we agree about the end goal, just disagree about the incremental steps to get there.)

joelpittet’s picture

Assigned: Unassigned » catch

@catch, I'm assigning to you because you would likely be able to help decide how this should work.

@Crell is

tentatively OK with doing something with getStyles() now on the understand that it changes after we get HtmlFragment carrying defined value objects that may help.

From me:

  • This patch isn't too drastic from what we have now in the template (which is important for themers @see #20 and #21
  • Removing RenderWrapper helps the beta blocker to remove auto-escape
  • Providing the HtmlPage to the template provides a bit of intersting ways to manipulate the public data on that object. Like looping through the metaElements or linkElements arrays.

So we are trading temporary for temporary with a few extra minor benefits. The question being "how does this sit with you?"

Fabianx’s picture

So in my book this would be **RTBC** and this all looks great, but if @Crell still disagrees with polluting the core HtmlPage.php, my take is to use a decorator that only exposes the right methods we want themers to use:

use Drupal\Core\Template\HtmlPageDecorator;
[...]
$variables['page'] = new HtmlPageDecorator($page);
[...]
class HtmlPageDecorator implements ... {

  protected $page;

  public function getStyles() {
  }
[...]
  etc.
}

This can then either call drupal_get_css directly or call up to the wrapped object - no pollution all the nice DX and TX (themer experience).

Very little changes from this patch and using a known pattern to 'represent an object in a different way to the user of it'.

joelpittet’s picture

@Fabianx thanks, yeah I was thinking proxy pattern, but decorator is more appropriate. We are waiting on @catch to chime in but I really like that decorator approach.

If @Crell is more comfortable with that approach I'd gladdly try to implement that instead.

Crell’s picture

I can deal with the decorator, but I think hearing from catch if he's OK with a temp set of methods while we sort out assets would be best before we do anything else. We still haven't heard from him.

Although Joel, I thought we had a plan coming out of DrupalCon, didn't we?

joelpittet’s picture

@Crell with my schedule ATM, I don't have the time to implement the Asset objects and deal with the Assetic vs Temp Asset Object mapping. The scope of that plan was a bit to vague and risky for me to take on.

catch’s picture

Fine with the patch as it is currently.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Per #31 I am gonna be bold and mark this RTBC then with the understanding that any presentation logic will be removed anyway, because we are moving into direction of making CSS/JS true asset objects.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 19: 2272279-die-RenderWrapper-19.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
8.63 KB
693 bytes

Rerolled; will re-RTBC. I did a quick diffdiff shows clearly the patch got broken by the String::checkPlain conversion as the only material difference is a use..String; line.

chx’s picture

Status: Needs review » Reviewed & tested by the community
catch’s picture

34: 2272279_34.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 34: 2272279_34.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
8.79 KB

Ok thanks @Catch, I've re-rolled what I have, I'll open up a follow-up to explore Decorator as @Fabianx suggested in #27 because I like that abstraction, but this quick fix does the deed and unblocks a couple other issues.

joelpittet’s picture

Added Decorator follow-up to related issues.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC, thanks @joelpittet

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

  • catch committed 03cc400 on 8.x
    Issue #2272279 by joelpittet, chx: Kill RenderWrapper class.
    
joelpittet’s picture

Thank you very much @Catch et al!

joelpittet’s picture

Status: Fixed » Closed (fixed)

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