Bring back metatag support for the HtmlPage object

+++ b/core/lib/Drupal/Core/Page/DefaultHtmlFragmentRenderer.php
@@ -82,7 +82,36 @@ public function preparePage(HtmlPage $page, &$page_array) {
+    // Display the html.html.twig's default mobile metatags for responsive design.

Display the who's what? html.html.twig? What's that?

Otherwise this looks almost exactly like what we pulled out of the HtmlPage issue, so I'm obviously +1 on bringing it back. :-)

Ahhh... Without context it looked like a service name. :-)

1. +++ b/core/includes/common.inc
   @@ -321,44 +316,23 @@ function drupal_add_html_head($data = NULL, $key = NULL) {
   -      'charset' => 'utf-8',
   ...
   -    // Security: This always has to be output first.
   -    '#weight' => -1000,
   
   +++ b/core/lib/Drupal/Core/Page/DefaultHtmlFragmentRenderer.php
   @@ -82,7 +82,36 @@ public function preparePage(HtmlPage $page, &$page_array) {
   +    $page->addMetatag(new Metatag(NULL, array(
   +      'name' => 'system_meta_content_type',
   +      'charset' => 'utf-8',
   +    )));
   

   'name' => 'charset',
   'charset' => 'utf-8',

   The 'name' is not supposed to be output. Did you test this?

   I can't seem to find where the negative weight / high priority is applied? The charset meta tag still needs to come first for security reasons, AFAIK.

2. +++ b/core/includes/common.inc
   @@ -321,44 +316,23 @@ function drupal_add_html_head($data = NULL, $key = NULL) {
   + *   If TRUE render the HEAD elements, otherwise eturn just the elements.
   

   Typo in "eturn"

3. +++ b/core/lib/Drupal/Core/Page/HeadElement.php
   @@ -72,6 +72,13 @@ public function setAttribute($key, $value) {
   +   * Gets all the attributes.
   +   */
   +  public function &getAttributes() {
   

   What does this @return?

   An array of raw attributes or an array of objects of a certain type?

4. +++ b/core/lib/Drupal/Core/Page/HtmlPage.php
   @@ -69,6 +83,52 @@ public function __construct($content = '', array $cache_info = array(), $title =
   +   * @return self
   ...
   +   * @return self
   
   +++ b/core/lib/Drupal/Core/Page/Metatag.php
   @@ -0,0 +1,83 @@
   +   * @return self
   ...
   +   * @return self
   

   $this

5. +++ b/core/lib/Drupal/Core/Page/HtmlPage.php
   @@ -69,6 +83,52 @@ public function __construct($content = '', array $cache_info = array(), $title =
   +  public function &links() {
   ...
   +  public function &metatags() {
   

   Can we rename these to have a 'get' method prefix like the other methods?

6. +++ b/core/lib/Drupal/Core/Page/Link.php
   @@ -0,0 +1,40 @@
   +/**
   +* @file
   +* Contains \Drupal\Core\Page\Link.
   +*/
   ...
   +/**
   +* Defines a link html HEAD element, which is defined by the href of the link.
   +*/
   

   Bogus phpDoc indentation.

7. +++ b/core/lib/Drupal/Core/Page/Link.php
   @@ -0,0 +1,40 @@
   +  * @param string $rel
   +  *   (optional) The link relationship. This is usually an IANA or
   +  *   Microformat-defined string. Defaults to an empty string
   ...
   + public function __construct($href, $rel = '', array $attributes = array()) {
   

   Why is 'rel' optional?

   According to http://www.w3.org/TR/html5/document-metadata.html#the-link-element both 'href' and 'rel' are required...?

   Shouldn't $rel come first in the signature?

8. +++ b/core/lib/Drupal/Core/Page/Metatag.php
   @@ -0,0 +1,83 @@
   +class Metatag extends HeadElement {
   

   Why is this class named "MetaTag", while the other class is named "Link" (as opposed to "LinkTag") ?

9. +++ b/core/lib/Drupal/Core/Page/Metatag.php
   @@ -0,0 +1,83 @@
   +  public function __construct($content = '', array $attributes = array()) {
   +    $this->attributes = $attributes + array(
   +      'content' => $content,
   +    );
   +  }
   

   This signature and processing looks a bit weird to me.

   Why are we enforcing a 'content' attribute onto each meta tag, if we mix it into attributes anyway?

10. +++ b/core/lib/Drupal/Core/Page/Metatag.php
    @@ -0,0 +1,83 @@
    +  public function setName($name) {
    ...
    +  public function setContent($content) {
    ...
    +  public function getName() {
    +    return $this->attributes['name'];
    +  }
    ...
    +  public function getContent() {
    +    return $this->attributes['content'];
    +  }
    

    These two do not necessarily exist.

    Shouldn't the MetaTag class much rather have generic set($name, $value) and get($name) methods for arbitrary meta element attributes?

11. +++ b/core/modules/system/tests/modules/system_module_test/lib/Drupal/system_module_test/EventSubscriber/HtmlPageSubscriber.php
    @@ -0,0 +1,45 @@
    +  public static function getSubscribedEvents() {
    +    // Execute between
    +    // \Drupal\Core\EventSubscriber\HtmlViewSubscriber::onHtmlFragment and
    +    // \Drupal\Core\EventSubscriber\HtmlViewSubscriber::onHtmlPage and
    +    $events[KernelEvents::VIEW][] = array('onHtmlPage', 60);
    

    This looks very fragile... what if there are 50 modules that need to add, change, or remove meta tags and links?

Re point 1, I don't think meta tags support fully arbitrary attributes. Dave Reid already reviewed the current class in the previous issue and had no further issues beyond what we already took care of. We're aiming for the common case, which is name and content and "sometimes other stuff".

Re point 11, then there's 50 modules that would need to register listeners. Which is no different than today; 50 modules would need to call drupal_add_html_head() from within hook_page_alter() or somewhere. It's no less fragile. And I've never in my life seen a site with 50 modules messing with header tags at once. I don't even think I've seen as many as a half-dozen.

Daniel, are you going to have time to respond to sun's other comments in #13?

Looking at the code again... shouldn't the meta and links methods be on HtmlFragment, not HtmlPage? Sam, Mark, and I have been discussing the next step and it involves an HtmlFragmentInterface that has just the getters, including those for sideband data (meta, links, assets, etc.) We may as well move the logic up there now, since it will be there later.

The NodeController::page() controller method specifies link tags for that page specifically. That gets translated to an HtmlFragment before being passed up.

A block that shows a view may also link to an RSS feed, which then gets added as a link, too.

All fragments need to be able to carry out of band data that will then get dissected and reassembled into a single HtmlPage object, with all of that out of band data together.

For now let's just go with the methods where we had them in the original issue before they were removed. When we introduce the interface we can rejigger exactly what goes where. (There will likely be multiple interfaces, but I want to get this moving forward.)

Incremental progress.

rerolled against head.

Eclipse

Ok, I have moved the methods as Crell asked for in #18. HtmlFragment is a parent class to HtmlPage so the interdiff is ridiculously straight forward.

Eclipse

Huzzah! Thanks, Kris!

I see, testbot just hates me...

Eclipse

drupal_pre_render_html() and other uses are not converted. We should at least open a follow-up?

We have feed icon support in the theme layer that consults drupal_get_feeds(). That's ignored here and it's going to result in different behaviour depending which mechanism is used.

This issue needs a issue summary and a change record.

-function hook_html_head_alter(&$head_elements) {
-  foreach ($head_elements as $key => $element) {
-    if (isset($element['#attributes']['rel']) && $element['#attributes']['rel'] == 'canonical') {
-      // I want a custom canonical URL.
-      $head_elements[$key]['#attributes']['href'] = mymodule_canonical_url();
-    }
-  }
-}

This hook is removed but HTML HEAD elements can (and still are in core) added via this mechanism. Those elements won't be alterable either via the event or this hook now.

Catch & I discussed this in -wscci and came to the conclusion that #33 is actually already taken care of, so that just leaves 31 & 32.

Eclipse

We discussed this in IRC between catch, dawehner, EclipseGc, and myself. The plan for dealing with feeds is thus:

1) Add a FeedLink extends LinkTag class.

2) Tweak template_preprocess_html to bridge drupal_add_feed the same way we bridge the other head methods.

3) Add an HtmlPage::getFeeds() method (only on that object, not fragment) that returns just those link objects that instanceof FeedLink. This can then be used by the theme layer for generating the RSS icons at the bottom of the page rather than calling drupal_get_feeds().

4) Add the same bridge code to HtmlControllerBase::createHtmlFragment to let feeds get added with the right object.

5) Commit this thing.

6) Factor the render->HtmlFragment code out of HtmlControllerBase to a service in a follow-up (to avoid creep): #2256365: Factor render->fragment code out to a service

7) Factor the read interface of HtmlFragment out to an interface of name TBD, follow-up: #2256373: Factor HtmlFragment out to an interface

The substantive changes here are good, just some weird artifacts left over. Note this still needs an issue summary.

1. +++ b/core/core.services.yml
   @@ -602,7 +602,7 @@ services:
   -    arguments: ['@content_negotiation', '@title_resolver', '@html_page_renderer', '@html_fragment_renderer', '@string_translation']
   +    arguments: ['@content_negotiation', '@title_resolver', '@html_page_renderer', '@html_fragment_renderer', '@url_generator']
   

   Is this a mistake after #2257745: Switch t() to $this->t() in ExceptionController?

2. +++ b/core/lib/Drupal/Core/Controller/ExceptionController.php
   @@ -26,6 +28,8 @@
   +  // @TODO The trait is just wrong for such a basic functionality!
   

   ...

3. +++ b/core/lib/Drupal/Core/EventSubscriber/HtmlViewSubscriber.php
   @@ -88,6 +88,11 @@ public function onHtmlPage(GetResponseForControllerResultEvent $event) {
   +      list($version, ) = explode('.', \Drupal::VERSION);
   
   +++ b/core/lib/Drupal/Core/Page/DefaultHtmlFragmentRenderer.php
   @@ -85,7 +85,36 @@ public function preparePage(HtmlPage $page, &$page_array) {
   +    list($version, ) = explode('.', \Drupal::VERSION);
   

   Do we want to use the 3rd param for explode, like explode('.', \Drupal::VERSION, 2);? Also, do we even need that comma in list()?

4. +++ b/core/lib/Drupal/Core/Page/HtmlFragment.php
   @@ -62,6 +76,52 @@ public function __construct($content = '', array $cache_info = array()) {
   +   *   The invoked object.
   ...
   +   *   The invoked object.
   

   We've been leaving these out now

Thanks! Would be great to quickly fix the following minor issues prior to commit:

1. +++ b/core/lib/Drupal/Core/Page/LinkElement.php
   @@ -0,0 +1,40 @@
   +/**
   +* @file
   +* Contains \Drupal\Core\Page\LinkElement.
   +*/
   

   Bogus indentation/formatting of phpDoc comment.

2. +++ b/core/lib/Drupal/Core/Page/MetaElement.php
   @@ -0,0 +1,83 @@
   +   * @return self
   +   *   The invoked object.
   ...
   +   * @return self
   +   *   The invoked object.
   

   $this, not self. We can skip the description.

+++ b/core/lib/Drupal/Core/Page/HtmlFragment.php
@@ -22,6 +22,20 @@
+  /**
+   * An array of Meta elements.
+   *
+   * @var array
+   */
+  protected $metatags = array();

@@ -62,6 +76,50 @@ public function __construct($content = '', array $cache_info = array()) {
+  /**
+   * Adds a Meta tag to the page.
+   *
+   * @param \Drupal\Core\Page\MetaElement $tag
+   *   A meta element to add.
+   *
+   * @return $this
+   */
+  public function addMetatag(MetaElement $tag) {
+    $this->metatags[] = $tag;
+    return $this;
+  }
+
+  /**
+   * Returns an array of all enqueued meta tags.
+   *
+   * @return \Drupal\Core\Page\MetaElement[]
+   */
+  public function &getMetatags() {
+    return $this->metatags;
+  }

If we're changing to use MetaElement then what should these functions & parameters be called?

And we still need an issue summary

Assuming the bot is OK...

+1 Looks great to me now.

+++ b/core/includes/theme.inc
@@ -2111,7 +2108,27 @@ function template_preprocess_page(&$variables) {
+    foreach ($page->getLinkElements() as $link) {

Minor, but this could be $page->getFeedLinks() no?

I'm not sure about putting the links/feeds onto the page object in preprocess, seems like it'd be better in a step just before preprocess, but not sure what step that should be... - maybe a followup? I realise this is a bc shim but feels quite far off actually removing that yet.

#61 makes sense.

I don't think the BC shim is better/worse in either place, so, meh.

Yes good point, shouldn't call something backwards compatibility if it's the only thing keeping an entire subsystem working.

I'm OK with this now but we need an explicit follow-up/plan to make it actually possible to remove the old deprecated stuff - that's not linked from the summary/related issues at all.

I updated the existing follow-up at #2256365: Factor render->fragment code out to a service to clarify that it should be a complete render->fragment converter, including #attached. That should be sufficient for #64.

This smells catch-ish.

+++ b/core/lib/Drupal/Core/Page/DefaultHtmlFragmentRenderer.php
@@ -87,6 +87,15 @@ public function preparePage(HtmlPage $page, &$page_array) {
+    // Sadly there are modules which still call drupal_add_feed directly, like

One more thing, this comment isn't describing the problem, the issue is in drupal_render_collect_attached() and that needing to be connected to HtmlFragment, rather than legacy calls to drupal_add_feed(). Views doesn't call drupal_add_feed() any more, it uses #attached['drupal_add_feed'].

I updated the comment on commit since otherwise this patch could end up breaking the record for most times RTBC without actually being RTBC, if it hasn't already.

+    // @todo: collect feed links from #attached rather than a static once
+    // http://drupal.org/node/2256365 is completed.