Follow-up from #2218117: Bring back metatag support for the HtmlPage object.

Problem/Motivation

HtmlFragment is a good first step, but to really drive it forward we should turn it into an interface.

Proposed resolution

That is, we need an interface that represents a page fragment or block, whose methods provide *read only* access to the content and metadata of that fragment. The starting point for that (and for the purposes of this issue, the ending point) is the getter methods of HtmlFragment.

The goal is to get to the point that Block plugins can implement this interface directly, which means that, to the page rendering pipeline it simply has one fragment/block interface object from the controller and a bunch of others that it got from the blocks system (or whatever layout tool, that's off topic)

This issue is mostly a moving-code-around issue, not a new-functionality-creating issue.

Remaining tasks

Possible names for this interface: HtmlFragmentInterface, BlockInterface. Discuss.

API changes

HtmlFragment implements the interface defined here.

For follow-up: Block plugins implement this interface, too. NOT for this issue.

CommentFileSizeAuthor
#19 interdiff.txt682 bytesCrell
#19 2256373-fragment-interface.patch7.31 KBCrell
#17 2256373-fragment-interface.patch7.31 KBCrell
#12 2256373-fragment-interface-12.patch7.24 KBjoelpittet
#12 interdiff.txt1.19 KBjoelpittet
#10 interdiff.txt740 bytesCrell
#10 2256373-fragment-interface.patch7.78 KBCrell
#7 2256373-fragment-interface.patch7.24 KBCrell
#5 2256373-fragment-interface.patch7.21 KBCrell
#2 2256373-fragment-interface.patch7.04 KBCrell

Comments

Crell’s picture

Crell commented
Issue tags: -Needs issue summary update
Crell’s picture

Crell commented
Status: Active » Needs review
Issue tags: -Needs change record
StatusFileSize
new 2256373-fragment-interface.patch7.04 KB

KISS. This should have no functional changes, just moving stuff around.

Crell’s picture

Crell commented

See also: #2267843: Split off CacheableTrait

dawehner’s picture

dawehner
Primary language German
commented 
  1. +++ b/core/lib/Drupal/Core/Page/HtmlFragmentInterface.php
@@ -0,0 +1,60 @@
+ * A domain object for a portion of an HTML page, including sideband data.

    We should explain what sideband data means in this context. 

  2. +++ b/core/lib/Drupal/Core/Page/HtmlFragmentInterface.php
@@ -0,0 +1,60 @@
\ No newline at end of file

    ups

Crell’s picture

Crell commented
StatusFileSize
new 2256373-fragment-interface.patch7.21 KB

Addresses #4.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Makes sense and looks almost ready to me; just some minor things:

  1. +++ b/core/lib/Drupal/Core/Page/HtmlFragment.php
@@ -12,14 +12,7 @@
-/**
- * Response object that contains variables for injection into the html template.
...
- */
-class HtmlFragment implements CacheableInterface {
+class HtmlFragment implements CacheableInterface, HtmlFragmentInterface {

    We've lost the phpDoc on the class?

    "A generic HTML fragment."

    ? 

  2. +++ b/core/lib/Drupal/Core/Page/HtmlFragmentInterface.php
@@ -0,0 +1,64 @@
+  public function getMetaElements();
...
+  public function hasTitle();
...
+  public function getContent();
...
+  public function getTitle();

    The order of methods on the interface looks quite random right now. Can we re-order them?

  3. +++ b/core/lib/Drupal/Core/Page/HtmlFragmentRendererInterface.php
@@ -24,7 +24,7 @@
-   * @param \Drupal\Core\Page\HtmlFragment $fragment
+   * @param HtmlFragmentInterface $fragment

    To my knowledge, we're not following PSR-5 (yet) and still using fully qualified resource names.

Crell’s picture

Crell commented
StatusFileSize
new 2256373-fragment-interface.patch7.24 KB

1 and 3 are artifacts of PHPStorm's refactor command. Corrected.

For point 2, the order was, I think, the order that those methods are in HtmlFragment. Meh. I've reordered them to something that I think makes sense, but let's not bikeshed that. I did not rearrange anything in HtmlFragment so as to not create unnecessary patch jitter.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Reviewed & tested by the community

Thanks!

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/lib/Drupal/Core/Page/DefaultHtmlFragmentRenderer.php
@@ -35,15 +36,18 @@ public function __construct(LanguageManager $language_manager) {
 
+    if ($fragment instanceof CacheableInterface) {
+      $page_content['main']['#cache'] = ['tags' => $fragment->getCacheTags()];
+    }
+

What's wrong with ['#cache']['tags']?

Otherwise looks OK.

Crell’s picture

Crell commented
Assigned: Unassigned » Crell
Status: Needs work » Needs review
StatusFileSize
new 2256373-fragment-interface.patch7.78 KB
new interdiff.txt740 bytes

Nothing. Just an editing quirk.

Freshly rebased patch attached.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Reviewed & tested by the community
joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
StatusFileSize
new interdiff.txt1.19 KB
new 2256373-fragment-interface-12.patch7.24 KB

Just removing the default.settings.php change that got added in there and a minor whitespace.
RTBC++

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented 
+++ b/core/lib/Drupal/Core/Page/HtmlFragmentInterface.php
@@ -0,0 +1,63 @@
+ * A domain object for a portion of an HTML page, including sideband data.
+ *
+ * Sideband data includes all non-HTML content relevant to a portion of a page.
+ * That includes required CSS files, Javascript files, HTML <head> elements
+ * such as links or meta tags, and so on.

Is sideband data used anywhere else to mean this? Google didn't turn up anything useful.

Crell’s picture

Crell commented

I don't know that there is a standard collective term for "that sort of stuff". "Sideband", I believe, comes from the telecomm industry and refers to information sent in "gaps" in the audio signal, mostly control information and is also where SMS messages live. (It's complicated audio frequency mechanics I don't fully understand.)

If there's a better name for that please suggest it, but please please don't bikeshed this simple issue on that as it's blocking at least 2 other issues at the moment. It's just in a comment so has no API impact.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

Using a term in a way that is not been used before and a term that you admit to not fully understanding in our documentation and me pointing this out is not bikeshedding. Documentation exists to add clarity and help humans interpret what something does. Semantically extending sideband for Drupal is probably not the way to go. You are not sending any information in the gaps.

Like is anything lost by changing "sideband" to "related" or "additional". For example,

+ * A domain object for a portion of an HTML page, including related data.
+ *
+ * Related data includes all non-HTML content relevant to a portion of a page.
+ * That includes required CSS files, Javascript files, HTML <head> elements
+ * such as links or meta tags, and so on.

"and so on" - this is very vague, we should be a bit more precise here.

catch’s picture

catch
he/him
Primary language English
commented

Also <head> elements are HTML, so it's not "non-HTML".

We could possibly use 'additional page-level data and assets' - the important thing to convey is that these are things which have to be applied to the page as it's rendered as a whole, as opposed to the specific fragment.

Crell’s picture

Crell commented
Status: Needs work » Needs review
StatusFileSize
new 2256373-fragment-interface.patch7.31 KB

Trying again on the comment... No interdiff because nothing changed other than that one docblock, which was a total rewrite.

dawehner’s picture

dawehner
Primary language German
commented

No interdiff because nothing changed other than that one docblock, which was a total rewrite.

And you really think that typing that sentence was less effort than just creating the interdiff?

  1. +++ b/core/lib/Drupal/Core/Page/HtmlFragment.php
@@ -13,13 +13,9 @@
+ * Basic mutable implementation of an HTML fragment.
...
+class HtmlFragment implements CacheableInterface, HtmlFragmentInterface {

+++ b/core/lib/Drupal/Core/Page/HtmlFragmentInterface.php
@@ -0,0 +1,66 @@
+interface HtmlFragmentInterface {

    Given that we consider using an html fragment as sort of main entry point for modules like metatag I doubt we want to have a immutable interface,
    or at least we should really explain why we want to do that, beside of functional programming purity. 

  2. +++ b/core/lib/Drupal/Core/Page/HtmlFragmentInterface.php
@@ -0,0 +1,66 @@
+ * A domain object for a portion of an HTML page, including related data.
...
+ * for example, required CSS files, Javascript files, link tags, meta tags, or
+ * the title of a page or page section.

    It is confusing to use OR, AND seems more appropriate tbh.

Crell’s picture

Crell commented
StatusFileSize
new 2256373-fragment-interface.patch7.31 KB
new interdiff.txt682 bytes

1. ... Functional programming purity has nothing to do with it. We want to be able to use this interface for blocks, too. Those will be returning the various related stuff they require via those methods, but the data won't come from setters but from whatever logic exists inside that class. That makes setters inappropriate for the base interface. (HtmlFragment the class still has them, of course, and I suppose if we really want we could add an interface for the setters for HtmlFragment to use as well but that's for another issue.)

2. As a native English speaker I don't see much difference here between and/or, but.. meh.

(Can we please get this committed before we reach the 3 WEEK mark for just splitting off an interface?)

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

to quote you: I am so out of things

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 008bf08 and pushed to 8.x. Thanks!

diff --git a/core/lib/Drupal/Core/Page/HtmlFragmentInterface.php b/core/lib/Drupal/Core/Page/HtmlFragmentInterface.php
index 983c1bd..687dacb 100644
--- a/core/lib/Drupal/Core/Page/HtmlFragmentInterface.php
+++ b/core/lib/Drupal/Core/Page/HtmlFragmentInterface.php
@@ -5,17 +5,15 @@
  * Contains \Drupal\Core\Page\HtmlFragmentInterface.
  */
 
-
 namespace Drupal\Core\Page;
 
-
 /**
  * A domain object for a portion of an HTML page, including related data.
  *
- * Related data includes any additional information relevant to a fragment
- * of HTML that would not be part of the HTML string itself. That includes,
- * for example, required CSS files, Javascript files, link tags, meta tags, and
- * the title of a page or page section.
+ * Related data includes any additional information relevant to a fragment of
+ * HTML that would not be part of the HTML string itself. That includes, for
+ * example, required CSS files, Javascript files, link tags, meta tags, and the
+ * title of a page or page section.
  */
 interface HtmlFragmentInterface {

Reflowed comment on commit

  • Commit 008bf08 on 8.x by alexpott: 
    Issue #2256373 by Crell, joelpittet: Factor HtmlFragment out to an...

Status: Fixed » Closed (fixed)

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