Expand the support of head elements to allow every common head element.

In case you need an arbitrary HTML noone stops you to expand the HeadElement class to implement your own and add it onto the page object, see https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Page!HeadElement....

Okay you are right, but I don't see why we cannot add support for it.

Are you sure you want to include aribtrary stuff? I cannot see why this should be allowed on https://developer.mozilla.org/en-US/docs/Web/HTML/Element/head

My concern is that we're pushing very hard to have properly structured domain objects now rather than a weird and impossible to document amalgam of arrays, strings, and bubblegum. The Twig folks are also pushing to even have strings wrapped up in objects: #1825952: Turn on twig autoescape by default. Allowing a method to add an arbitrary HeadElement class or subclass, and making sure that those are also then rendered and returned, seems reasonable to me.

Allowing arbitrary unfiltered strings to be injected into the HTML head sounds very worrisome and very dangerous. It would also be completely bypassing all of Drupal's automation around such things, making this issue related to #2298551: Documentation updates adding/adjusting inline/external JS in themes.

What's the use case for "paste this arbitrary snippet of HTML" that you would want to be doing at a module HtmlPage domain object level, rather than just opening up html.html.twig and pasting it in there?

Well, a generic HeadElement would at least give us pre-parsed data that we can safely escape; much like the SQL query builders give us structured data that we know how to stringify and escape without relying on regexes. It's entirely arbitrary strings that I am scared of.

That said, the number of legal HTML head tags is small and finite. Could we just add any missing element types we have and call it a day?

Started some work on it.

@crell See tell in IRC

1. +++ b/core/lib/Drupal/Core/Page/HeadElement.php
   @@ -48,7 +53,12 @@ public function __toString() {
   diff --git a/core/lib/Drupal/Core/Page/HtmlFragment.php b/core/lib/Drupal/Core/Page/HtmlFragment.php
   
   diff --git a/core/lib/Drupal/Core/Page/HtmlFragment.php b/core/lib/Drupal/Core/Page/HtmlFragment.php
   index b3c55d8..7bc052b 100644
   
   index b3c55d8..7bc052b 100644
   --- a/core/lib/Drupal/Core/Page/HtmlFragment.php
   
   --- a/core/lib/Drupal/Core/Page/HtmlFragment.php
   +++ b/core/lib/Drupal/Core/Page/HtmlFragment.php
   
   +++ b/core/lib/Drupal/Core/Page/HtmlFragment.php
   +++ b/core/lib/Drupal/Core/Page/HtmlFragment.php
   @@ -125,6 +125,12 @@ public function &getMetaElements() {
   
   @@ -125,6 +125,12 @@ public function &getMetaElements() {
      }
    
      /**
   +   * {@inheritdoc}
   +   */
   +  public function addElement(HeadElement $element) {
   +  }
   +
   +  /**
   

   This isn't in the interface yet, it looks like. And shouldn't it actually do something? :-)

2. +++ b/core/lib/Drupal/Core/Page/TitleElement.php
   @@ -0,0 +1,30 @@
   +class TitleElement extends HeadElement {
   

   I think this one is unnecessary as we already have a method for the title on HtmlFragment/HtmlPage directly.

Thanks for the review!!

Yeah right. I think the getElements method should return all HEAD elements.

This time with much less failures.

Some less test failures.

+++ b/core/lib/Drupal/Core/Page/NoscriptElement.php
@@ -0,0 +1,31 @@
+class NoscriptElement extends HeadElement {

noscript tags are supposed to wrap other tags. Making it its own tag seems problematic. It's also unnecessary, as HeadElement already has a noscript flag on it. That makes a stand-alone class for it unnecessary.

+++ b/core/lib/Drupal/Core/Page/HtmlFragmentInterface.php
@@ -63,4 +63,9 @@ public function getFeedLinkElements();
+  /**
+   * Returns an array of all other HEAD elements.
+   */
+  public function getElements();

Except the implementation is not returning "all other HEAD elements". It's returning all HEAD elements, period. I'm OK with that, but the comment should reflect it.

I think other than those two points this is good to go. Thanks, dawehner!

@crell
Yeah right, this is just a leftover from the initial implementation.

One thing I am not really sure how to solve is, how we can ensure that passing by reference works for both link elements but also all elements. You could
store just a plain array of elements for getElements() but then the filtering for getLinkElements() would destroy it again. What about just supporting removing for getElements()?

Hm. Well the objects pass by handle. That gives two possibilities:

1) Use collection object rather than an array, so it always returns "by reference" (scare quotes intended).

2) Have getElements() create a new array of all objects that aggregates everything from the keyed array, then return that by reference. You'd still get a handle to the original objects, just not through the original array.

The better question, though, is what the use case is for getElements() returning by reference. For CSS and JS it would be to allow alters, essentially, specifically for #2334225: Move CSS/JS preprocessing to a View listener. But is there a use case for by-reference of all objects? I'm not clear that there is. For getting all objects period, the use case is the page template. Although I don't know if themers would even want that rather than doing them separately. :-)

The better question, though, is what the use case is for getElements() returning by reference. For CSS and JS it would be to allow alters, essentially, specifically for #2334225: Move CSS/JS preprocessing to a View listener. But is there a use case for by-reference of all objects? I'm not clear that there is. For getting all objects period, the use case is the page template. Although I don't know if themers would even want that rather than doing them separately. :-)

Well, at least at the moment it seemed to be common to reorder to metatags itself, why should allow that for every HEAD element?

What value is there to reordering metatags? Content type and encoding http-equivs should come first for browser parsing, but beyond that who cares? Does it impact anything?

More to the point, I'm leaning toward leaving it out at this point and getting the rest of this in. It's already a step forward and I don't want to hold up the issue on figuring out where we may want to possibly do that kind of fancy-pants reordering, especially since that opens up all kinds of other sticky questions.

Well, if we don't add support here, people will complain as metatags do have support for it already.

I'm quite sure but I don't thing meta tags ever need to be sorted. (Will grep contrib in d7 to be sure).

Questions that came to mind:

1. +++ b/core/lib/Drupal/Core/Page/BaseElement.php
   @@ -0,0 +1,36 @@
   +class BaseElement extends HeadElement {
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  protected $element = 'base';
   

   Is this needed? It's not used in the patch. I'm guessing this is the tag and would be in the 'other', category.

   Likely it is needed, just asking because if so, I may want to check other 'HeadElement' that may need to be added.

2. +++ b/core/lib/Drupal/Core/Page/HeadElement.php
   @@ -51,7 +56,12 @@ public function __toString() {
   -    $string = "<{$this->element}{$rendered} />";
   +    if (!isset($value)) {
   +      $string = "<{$this->element}{$rendered} />";
   +    }
   +    else {
   +      $string = "<{$this->element}{$rendered}>{$this->value}</{$this->element}>";
   +    }
        if ($this->noScript) {
          $string = "<noscript>$string</noscript>";
        }
   

   Can we move this back to #type=>html_tag or can we remove html_tag, they are competing for the exact same markup.

I am fine with removing #type=>html_tag.

1. +++ b/core/lib/Drupal/Core/Page/CommandElement.php
   @@ -0,0 +1,38 @@
   +      'type' => $type,
   +      'label' => $label,
   +      'icon' => $icon,
   +      'onclick' => $onclick,
   

   These variables are not defined...? They should be default values for the command element (I didn't even know that existed), possibly overridden by the constructor params.

2. +++ b/core/lib/Drupal/Core/Page/HtmlFragment.php
   @@ -20,18 +20,25 @@ class HtmlFragment implements CacheableInterface, HtmlFragmentInterface {
      /**
   +   * An array of other HEAD elements.
   +   *
   +   * @var \Drupal\Core\Page\HeadElement[]
   +   */
   +  protected $elements;
   

   This is now being used as the only elements property. Which means

   1) The description is wrong.
   2) The @var is wrong, since it's an array of arrays of head elements.
   3) We should document that structure.
   4) It should be called $headElements to make it clear that it's not arbitrary HTML elements.
   5) We should remove $metatags and $links.

+++ b/core/lib/Drupal/Core/Page/CommandElement.php
@@ -0,0 +1,39 @@
+  public function __construct($href, $target = NULL) {
+    $this->attributes = array(
+      'type' => 'command',
+      'label' => '',
+      'icon' => '',
+    );
+  }

Target is not a property of Command. The optional parameters should be label and icon, I wager, with defaults of '', and then specify those in the attributes array.

(Also, let's go ahead and use [ ] here instead of array(), just to be pedantic. :-) )

This is fine for me as it solves the problem people head with it. Would be great though of metatags module would be converted already.

Somehow I missed this comment on https://developer.mozilla.org/en-US/docs/Web/HTML/Element/command regarding command element -
This feature is obsolete. Although it may still work in some browsers, its use is discouraged since it could be removed at any time. Try to avoid using it.

I have removed CommandElement.php so it addresses the concern in https://www.drupal.org/node/2296951#comment-9196753.

Looks good to me.

Thanks, kgoel!

The issue summary is not update to date with the current patch.

1. +++ b/core/lib/Drupal/Core/Page/BaseElement.php
   @@ -0,0 +1,36 @@
   +class BaseElement extends HeadElement {
   

   This functionality is untested.

2. +++ b/core/lib/Drupal/Core/Page/HtmlFragment.php
   @@ -120,7 +119,30 @@ public function addMetaElement(MetaElement $meta) {
   +  public function &getElements() {
   
   +++ b/core/lib/Drupal/Core/Page/HtmlFragmentInterface.php
   @@ -63,4 +63,11 @@ public function getFeedLinkElements();
   +   * Returns an array of all HEAD elements.
   ...
   +  public function getElements();
   

   The implements don't match and the docs don't mention the elements are returned by ref.

Also we probably have some CRs to update after this. We need to document that we no longer support adding arbitrary stuff to HEAD

Due to #2350943: [Meta] Untangle Drupal 8 page rendering, this is effectively a won't-fix now, I'm afraid.