Initial patch is a split from #137 on parent issue.
https://www.drupal.org/files/issues/convert-page-elements-blocks-507488-...

Problem/motivation

Drupal 7 has most global site and page elements hardwired into templates, and gives no control to users to move them around. All the page elements (title, tabs, actions, breadcrumb, messages) are hardwired to core and contrib themes alike. We should stop special-casing these, so they can be moved around and selectively hidden or replaced as needed. This is the last remaining page variable to be converted into a block: doing so is a big win for consistency.

Proposed solution

  • Introduce title block that can be moved around.
  • Remove special case item from themes.

Much like the main content block the title block also for itself is not cached, as its cache contexts would be all possible combinations for all pages, aka. it would not be cacheable, so don't pretend to do so.

Related issues

Split off from #507488: Convert page elements (local tasks, actions) into blocks

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because it's an improvement, not a fix.
Issue priority Major because this shouldn't hold up release, but is rather important for the Themer Experience (no more lots of special casing in the page template) for Drupal 8.
Prioritized changes The main goal of this issue is TX, as this is the only remaining page variable to be converted to a block.
Disruption This provides an upgrade path that places a title block in all themes. If a page template uses special classes and markup for the page title, there could be disruption, however the change should be obvious and a message in the upgrade path explains the change.
CommentFileSizeAuthor
#322 interdiff.txt886 bytesWim Leers
#322 2476947-322-convert-page-title-block.patch82.2 KBWim Leers
#318 interdiff-315-318.txt540 bytesRainbowArray
#318 2476947-318-convert-page-title-block.patch81.42 KBRainbowArray
#315 2476947-315-convert-page-title-block.patch81.42 KBRainbowArray
#310 2476947-310-convert-page-title-block.patch81.42 KBRainbowArray
#301 interdiff-2476947-293-301.txt3.38 KBRainbowArray
#301 2476947-301-convert-page-title-block.patch81.36 KBRainbowArray
#293 interdiff-2476947-288-293.txt2.41 KBRainbowArray
#293 2476947-293-convert-page-title-block.patch81.34 KBRainbowArray
#288 interdiff-2476947-285to288.txt1.31 KBdavidhernandez
#288 convert_title_page-2476947-288.patch81.47 KBdavidhernandez
#285 2476947-285-convert-page-title-block.patch80.8 KBRainbowArray
#283 interdiff.txt3.59 KBstar-szr
#283 convert_title_page-2476947-283.patch80.81 KBstar-szr
#279 interdiff-2476947-268-278.txt560 bytesRainbowArray
#279 2476947-278-convert-title-block.patch80.82 KBRainbowArray
#274 Screenshot 2015-09-19 21.04.52.jpg267.83 KBLewisNyman
#274 Appearance___Site-Install.png593.67 KBLewisNyman
#274 Appearance___Site-Install.png589 KBLewisNyman
#271 interdiff-2476947-260-268.txt562 bytesRainbowArray
#271 2476947-268-convert-page-title-block.patch80.27 KBRainbowArray
#262 TitleBlockInSeven.png9.33 KBmgifford
#260 interdiff-2476947-257-260.txt1.17 KBRainbowArray
#260 2476947-260-convert-page-title-block.patch80.27 KBRainbowArray
#257 interdiff-2476947-253-257.txt1.16 KBRainbowArray
#257 2476947-257-convert-page-title-block.patch81.06 KBRainbowArray
#253 interdiff-2476947-247-252.txt2.06 KBRainbowArray
#253 2476947-252-convert-page-title-block.patch81.47 KBRainbowArray
#248 interdiff-2476947-245-247.txt18.24 KBRainbowArray
#248 2476947-247-convert-page-title-block.patch80.08 KBRainbowArray
#245 interdiff.txt7.37 KBWim Leers
#245 convert_page_title_to_block-2476947-245.patch61.84 KBWim Leers
#236 interdiff-2476947-236-236FAIL.txt640 bytesRainbowArray
#236 interdiff-2476947-235-236.txt934 bytesRainbowArray
#236 2476947-236-convert-page-title-block.patch62.23 KBRainbowArray
#236 2476947-236-convert-page-title-block-FAIL.patch62.86 KBRainbowArray
#235 interdiff.txt3.94 KBWim Leers
#235 convert_page_title_to_block-2476947-235.patch61.61 KBWim Leers
#231 interdiff-2476947-230-231.txt2.06 KBRainbowArray
#231 2476947-231-convert-page-title-block.patch61.1 KBRainbowArray
#230 interdiff.txt8.42 KBWim Leers
#230 convert_page_title_to_block-2476947-230.patch60.35 KBWim Leers
#228 interdiff-2547159-218-228.txt640 bytesRainbowArray
#228 2547159-228-convert-page-title-block.patch62.29 KBRainbowArray
#226 Screen Shot 2015-09-14 at 10.31.16 PM.png87.83 KBRainbowArray
#226 Screen Shot 2015-09-14 at 10.25.33 PM.png124.95 KBRainbowArray
#224 bartik-blockdisabled.png130.17 KBdavidhernandez
#223 linkscreenshot.png9.21 KBdavidhernandez
#218 interdiff.txt1.07 KBWim Leers
#218 convert_page_title_to_block-2476947-218.patch62.92 KBWim Leers
#212 interdiff.txt5.93 KBWim Leers
#212 convert_page_title_to_block-2476947-211.patch63.87 KBWim Leers
#212 convert_page_title_to_block-2476947-211-FAIL.patch65.83 KBWim Leers
#210 xhprof runs.zip227.52 KBWim Leers
#210 ab runs.zip7.51 KBWim Leers
#210 histogram_interleaved.png10.92 KBWim Leers
#206 interdiff.txt2.44 KBWim Leers
#206 convert_page_title_to_block-2476947-206.patch63 KBWim Leers
#204 interdiff-2476947-201-204.txt669 bytesRainbowArray
#204 2476947-204-convert-page-title-block.patch61.67 KBRainbowArray
#201 interdiff-2476947-200-201.txt2.86 KBRainbowArray
#201 2476947-201-convert-page-title-block.patch61.64 KBRainbowArray
#200 interdiff-2476947-180-200.txt5.68 KBRainbowArray
#200 2476947-200-convert-page-title-block.patch61.53 KBRainbowArray
#198 install-configure.png263.65 KBdavidhernandez
#197 runningupdateafterpatch.png217.41 KBdavidhernandez
#197 prepatch-install.png189.65 KBdavidhernandez
#196 after-install.png79.47 KBRainbowArray
#196 before-install.png65.66 KBRainbowArray
#192 bartik-maintenance.png82.84 KBdavidhernandez
#192 bartik-before-maintenance.png97.31 KBdavidhernandez
#189 install-title.png135.59 KBdavidhernandez
#189 bartik-title.png156.39 KBdavidhernandez
#189 seven-title.png183.95 KBdavidhernandez
#189 classy-node-title.png143.74 KBdavidhernandez
#189 stark-node-title.png100.62 KBdavidhernandez
#180 interdiff.txt2.99 KBWim Leers
#180 convert_page_title_to_block-2476947-180.patch61.37 KBWim Leers
#174 interdiff.txt7.11 KBWim Leers
#174 convert_page_title_to_block-2476947-174.patch60.04 KBWim Leers
#172 interdiff.txt21.59 KBWim Leers
#172 convert_page_title_to_block-2476947-172.patch59.63 KBWim Leers
#171 interdiff.txt3.71 KBWim Leers
#171 convert_page_title_to_block-2476947-171.patch78.83 KBWim Leers
#170 interdiff.txt8.22 KBWim Leers
#170 convert_page_title_to_block-2476947-170.patch78.73 KBWim Leers
#169 interdiff.txt1.45 KBWim Leers
#168 interdiff.txt8.07 KBWim Leers
#168 convert_page_title_to_block-2476947-168.patch83.93 KBWim Leers
#167 interdiff.txt8.07 KBWim Leers
#167 convert_page_title_to_block-2476947-167.patch85.27 KBWim Leers
#165 interdiff-2476947-163-164.txt11.34 KBRainbowArray
#165 2476947-164-convert-page-title-block.patch84.89 KBRainbowArray
#164 interdiff-2476947-163-164.txt11.34 KBRainbowArray
#164 2476947-164-convert-page-title-block.patch84.89 KBRainbowArray
#163 interdiff-2476947-162-163.txt10.95 KBRainbowArray
#163 2476947-163-convert-page-title-block.patch75.61 KBRainbowArray
#162 interdiff-converttitle-156to162.txt663 bytesdavidhernandez
#162 convert_title_page-2476947-162.patch75.68 KBdavidhernandez
#156 interdiff.txt2.78 KBWim Leers
#156 convert_page_title_to_block-2476947-156.patch75.03 KBWim Leers
#155 interdiff.txt1007 bytesWim Leers
#155 convert_page_title_to_block-2476947-155.patch73.52 KBWim Leers
#147 interdiff.txt6.51 KBWim Leers
#147 convert_page_title_to_block-2476947-145.patch72.66 KBWim Leers
#144 screenshot-converttitle.png40.14 KBdavidhernandez
#144 interdiff-pagetitle-134to144.txt1.69 KBdavidhernandez
#144 convert_title_page-2476947-144.patch67.7 KBdavidhernandez
#134 interdiff.txt2.96 KBWim Leers
#134 convert_page_title_to_block-2476947-134.patch66.8 KBWim Leers
#133 interdiff.txt930 bytesWim Leers
#133 convert_page_title_to_block-2476947-133.patch66.92 KBWim Leers
#130 interdiff.txt1.77 KBWim Leers
#130 convert_page_title_to_block-2476947-130.patch66.92 KBWim Leers
#129 interdiff.txt1.84 KBWim Leers
#129 convert_page_title_to_block-2476947-129.patch66.68 KBWim Leers
#126 solution-b-interdiff.txt10.98 KBWim Leers
#126 solution-a-interdiff.txt3.57 KBWim Leers
#126 interdiff.txt1.07 KBWim Leers
#126 convert_page_title_to_block-2476947-126.patch65.47 KBWim Leers
#123 interdiff.txt1.61 KBWim Leers
#123 convert_page_title_to_block-2476947-123.patch64.83 KBWim Leers
#116 interdiff-2476947-113-116.txt1.67 KBRainbowArray
#116 2476947-116-convert-page-title-block.patch65.71 KBRainbowArray
#113 interdiff.txt2.05 KBjoelpittet
#113 convert_title_page-2476947-113.patch65.63 KBjoelpittet
#107 interdiff-2476947-98-107.txt7.52 KBRainbowArray
#107 2476947-107-convert-page-title-block.patch66.53 KBRainbowArray
#98 convert_title_page-2476947-98.patch72.52 KBlauriii
#95 convert_title_page-2476947-94.patch66.33 KBlauriii
#95 interdiff.txt961 byteslauriii
#91 convert_title_page-2476947-91.patch66.5 KBlauriii
#85 interdiff-2476947-76-reroll-85.txt1.62 KBRainbowArray
#85 2476947-85-convert-page-title-block.patch67.09 KBRainbowArray
#84 2476947-76-reroll.patch66.86 KBRainbowArray
#76 interdiff.txt1.44 KBWim Leers
#76 2476947-76-convert-page-title-block.patch67.13 KBWim Leers
#75 interdiff.txt1.45 KBWim Leers
#75 2476947-75-convert-page-title-block.patch67.13 KBWim Leers
#74 interdiff.txt1.67 KBWim Leers
#74 2476947-74-convert-page-title-block.patch65.79 KBWim Leers
#73 interdiff.txt15.57 KBWim Leers
#73 2476947-73-convert-page-title-block.patch65.32 KBWim Leers
#70 interdiff.txt1.47 KBWim Leers
#70 2476947-70-convert-page-title-block.patch50.67 KBWim Leers
#68 interdiff.txt8.64 KBWim Leers
#68 2476947-68-convert-page-title-block.patch49.32 KBWim Leers
#67 interdiff.txt7.9 KBWim Leers
#67 2476947-67-convert-page-title-block.patch46.28 KBWim Leers
#64 interdiff-2476947-62-64.txt13.98 KBRainbowArray
#64 2476947-64-convert-page-title-block.patch41.61 KBRainbowArray
#62 interdiff-2476947-59-62.txt619 bytesRainbowArray
#62 2476947-62-convert-page-title-block.patch27.63 KBRainbowArray
#59 interdiff.txt5.68 KBWim Leers
#59 2476947-59-convert-page-title-block.patch27.02 KBWim Leers
#54 interdiff-2476947-52-54.txt623 bytesRainbowArray
#54 2476947-54-convert-page-title-block.patch26.04 KBRainbowArray
#52 interdiff-2476947-50-52.txt1.64 KBRainbowArray
#52 2476947-52-convert-page-title-block.patch26.65 KBRainbowArray
#50 interdiff-2476947-48-50.txt632 bytesRainbowArray
#50 2476947-50-convert-page-title-block.patch26.62 KBRainbowArray
#48 interdiff-2476947-43-48.txt1.8 KBRainbowArray
#48 2476947-48-convert-page-title-block.patch26.61 KBRainbowArray
#43 interdiff-2476947-40-43.txt11.59 KBRainbowArray
#43 2476947-43-convert-page-title-block.patch26.59 KBRainbowArray
#40 interdiff-2476947-37-40.txt26.18 KBRainbowArray
#40 2476947-40-convert-page-title-block.patch26 KBRainbowArray
#37 interdiff-2476947-34-37.txt722 bytesRainbowArray
#37 2476947-37-convert-page-title-block.patch26.18 KBRainbowArray
#35 interdiff-2476947-32-34.txt1.49 KBRainbowArray
#35 2476947-34-convert-page-title-block.patch26.01 KBRainbowArray
#32 interdiff-2476947-31-32.txt1.61 KBRainbowArray
#32 2476947-32-convert-page-title-block.patch26.02 KBRainbowArray
#31 2476947-31-convert-page-title-block.patch25.9 KBRainbowArray
#28 interdiff-2476947-26-28.txt5.64 KBRainbowArray
#28 2476947-28-convert-page-title-block.patch25.9 KBRainbowArray
#26 interdiff-2476947-23-26.txt600 bytesRainbowArray
#26 2476947-26-convert-page-title-block.patch24.16 KBRainbowArray
#23 2476947-convert-page-title-block.patch23.57 KBRainbowArray
#15 convert_title_page-2476947-15.patch23.59 KBJeroenT
#15 interdiff-13-15.txt447 bytesJeroenT
#13 2476947-13.patch23.06 KBandypost
#13 interdiff.txt1.69 KBandypost
#7 2476947-7.patch23.59 KBrpayanm
#3 interdiff.txt5.49 KBWim Leers
#3 title_as_block-2476947-3.patch24.99 KBWim Leers
convert-title-to-block-1.patch22.1 KBManuel Garcia
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

I'll take this on. Not assigning to me yet in case somebody gets to it before me. Please assign it to yourself so I know you're working on it.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
24.99 KB
5.49 KB

Here's an initial bit of work. Fixes the bug where you end up with <h1><div><h1>title…, which obviously is not quite intended :) Also fixing some nits, and one test's failures.

The biggest remaining thing is the title prefix/suffix stuff, which is very much broken in this patch.

Wim Leers’s picture

Needs to be rerolled after #2369987: Remove SafeMarkup::set() from 'head' title on template_preprocess_html landed. And of course, the 158 failures still need to be fixed.

Gábor Hojtsy’s picture

rpayanm’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
23.59 KB

Let me try.

bill richardson’s picture

Patch needs reroll

andypost’s picture

Issue tags: +Needs reroll
andypost’s picture

now we have 2 title_suffix and title_prefix, one in block and another in page.html.twig - suppose shortcuts needs other way to attach

+++ b/core/themes/bartik/bartik.theme
@@ -43,23 +43,6 @@ function bartik_preprocess_html(&$variables) {
-  // Since the title and the shortcut link are both block level elements,
-  // positioning them next to each other is much simpler with a wrapper div.
-  if (!empty($variables['title_suffix']['add_or_remove_shortcut']) && $variables['title']) {
-    // Add a wrapper div using the title_prefix and title_suffix render
-    // elements.

+++ b/core/themes/bartik/templates/page.html.twig
@@ -123,12 +123,7 @@
-            {{ title_prefix }}
...
+            {{ page.title }}
             {{ title_suffix }}

+++ b/core/themes/classy/templates/layout/page.html.twig
@@ -105,9 +105,7 @@
       {{ title_prefix }}
-      {% if title %}
-        <h1>{{ title }}</h1>
-      {% endif %}
+      {{ page.title }}
       {{ title_suffix }}

+++ b/core/themes/seven/templates/page.html.twig
@@ -60,8 +60,8 @@
       {{ title_prefix }}
-      {% if title %}
-        <h1 class="page-title">{{ title }}</h1>
+      {% if page.title %}
+        <h1 class="page-title">{{ page.title }}</h1>
       {% endif %}
       {{ title_suffix }}

this should be fixed,

Wim Leers’s picture

Indeed, I said the same in #1:

The biggest remaining thing is the title prefix/suffix stuff, which is very much broken in this patch.

andypost’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
1.69 KB
23.06 KB

Re-roll and fix #11

Still need to decide for #12

JeroenT’s picture

Status: Needs work » Needs review
FileSize
447 bytes
23.59 KB

Not sure this is the right way, but this should fix a lot of the failing tests.

Patch attached.

JeroenT’s picture

.

Wim Leers’s picture

#15 That's unfortunately not the right way. It should be the right way, but it isn't, because we still allow $page['#title'] to override the route's _title or _title_callback. The primary reason being Views, which has super-dynamic titles that may require lots of expensive computations to determine the title. We don't want to run those computations twice.

(See #2403359: Use _title and _title_callback where possible and #2359901: Discourage $main_content['#title'] in favor of route titles and title callbacks for more about that.)

emma.maria’s picture

Issue tags: +Bartik, +Seven
RainbowArray’s picture

Status: Needs work » Needs review

Here's a straight-up reroll of #15 so we have a starting point to fixing this. The local actions and tasks block conversion in #507488: Convert page elements (local tasks, actions) into blocks is getting close, so this is the last page variable conversion. Would be great to not have one oddball page variable in D8.

RainbowArray’s picture

And the actual patch.

Manuel Garcia’s picture

Title: Convert "title" page element into blocks » Convert "title" page element into a block
RainbowArray’s picture

Status: Needs work » Needs review
FileSize
24.16 KB
600 bytes

It seems like some of the errors might be caused because the page title block wasn't in the install profile for Stark.

I'm not really sure what needs to be done with the title prefix and suffix, or what the shortcut title resolver should look like, but if I get this closer, maybe somebody else will be able to spot the quick fixes necessary to get this across the finish line.

RainbowArray’s picture

Well, the exciting thing is we probably have more errors and exceptions because the title block is actually showing up in Stark now.

This patch should take care of the doubling up of title_prefix and title_suffix. I've moved those inside the block in all cases, which seems much more straightforward. There was some weirdness going on with Seven's page.html.twig where the region was declared inside the h1 elements. Those have been moved into the blocks.

Maybe this will clear up some bugs?

It seems like the big issue is that SimplePageVariant and BlockPageVariant have arrays inside $this->title when those should be strings.

HtmlRenderer is one of the places that gets set, either from $main_content['title'] or titleResolver->getTitle, which does appear to return a string.

Ah.... I think I found it. BlockPageVariant can get the title from TitleBlockPluginInterface, and SystemPageTitleBlock sets title to... wait for it... an array. With the title inside #markup.

So maybe it just shouldn't do that? I'll try that and see what happens.

RainbowArray’s picture

Status: Needs work » Needs review
RainbowArray’s picture

RainbowArray’s picture

Status: Needs work » Needs review
FileSize
26.02 KB
1.61 KB

I feel like there's a decent chance I'm doing something wrong here. Trying to render the title #markup in order to avoid the array to string conversions. Guessing this is probably not the best way to handle this problem.

RainbowArray’s picture

Status: Needs work » Needs review

Took some reading of the render docs, but I'm thinking this might help with the exceptions. Using renderPlain instead of render, since it should just be #markup in the $title, and there was a bug in how I was title #markup in BlockPage Variant.

I still expect there will be a lot of errors. Ultimately I think we'll need to enable block module and place the title block in many of the tests with errors, because that won't be placed by default. I'm hoping we can get the number of test errors down at least somewhat, though, because placing the title block up to 6,786 times seems... not appealing. There may be a cascade of errors from one missing title block, so hopefully it's a lot fewer times than that. Example code of how we can do this somewhat efficiently in #2562987: Enable blocks in testing profile and/or Classy.

RainbowArray’s picture

RainbowArray’s picture

This should fix the preprocess exceptions. Hopefully moving in the right direction.

RainbowArray’s picture

Status: Needs work » Needs review

Previously, the page title was just in the page template, and we put h1 tags around in the system version of that template, and all was well.

However, by moving the title into the block template, we can still put the h1 tags around the title in that block's template, cool cool cool, but what about when blocks aren't enabled? In that case, rather than using BlockPageVariant, we're using SimplePageVariant. So the initial patch had used #markup in the build for that variant to insert the h1 tags. However... everything in core was expecting the page title to just be a simple string, not a render array. So... oodles of errors.

Just for kicks, moving the title back to a simple string for both SimplePageVariant and BlockPageVariant. That means there are no h1 tags for SimplePageVariant, so probably a new selection of test errors await.

The page template now no longer has a page title variable. It has a page.title region, right? So when we're setting page's title key in the build, I guess that can be reused elsewhere even if it doesn't show up in the template.

It seems pretty nasty to hardcode those h1 tags into SimplePageVariant now that I think about it. If for some reason somebody does have blocks all the way turned off and is just getting the SimplePageVariant, those hardcoded h1 tags would be super hard to override in the theme level. That seems really counter-productive for theming.

RainbowArray’s picture

ndf’s picture

One conceptual thing, imho we should not create a specific theme region for the title-block.
I guess it should be added to the 'Content' region, and probably rendered first/on-top by default, just before the 'Main page content'-block.

There are some related discussions about this regarding to menu's and message's:

Also, naming a region after the content within the region is not best practice for themes. You can add any content to the primary menu region, and then this name becomes even more confusing.

In we must introduce a new region as needed to place the block, we could go for 'Before Content'.

RainbowArray’s picture

I think I'm finally beginning to understand, and the region tip was very helpful. Thanks, ndf.

The #markup for the title is creating a render array for the title, and when you look at how that is being done for messages, something similar is done so that messages show up even there is no placed messages block. However, that messages render array is placed inside the content, and I think it makes sense to do that here as well. I've set the weight of that array so that it appears below the messages.

I think that should clear things up considerably.

I also pulled out the title markup from the maintenance and install templates. One thing to note there is that Bartik had an id="page-title" on the h1 in its maintenance template. I'm not sure that it would be that straightforward to send a different block template for the page title just to Bartik's maintenance page. I don't see that ID being used in Bartik's CSS, though, so hopefully it's fine?

The highlighted region also sometimes appeared below the title, sometimes above. There are also two highlighted region placements in system page.html.twig for some reason. I'm pretty sure there's an issue to get rid of the highlighted region, though, so not worrying too much about that now.

Crossing my fingers and hoping I understand this now. I've definitely been wrong about that before.

RainbowArray’s picture

Oh, also, I think the reason why there were so many failures before was at least in part because the $build placement of the title in a #markup render array was going into a title key on the page variants, but there wasn't a 'title' key: the region name was page_title. That's a moot point if we're not creating a title region anymore, but at least it explains why things may have been so broken.

emma.maria’s picture

Thank you for removing the title region from Bartik in #43. I fully agree with #42. Bartik's page title is always printed within the content region. I will take a look at what is happening with the maintenance page when I do some manual testing.

Seven's page title however sits high up on the page above the header region, so it may well need a new region, but please do not call it Title. @lewisnyman can help provide direction on this.

RainbowArray’s picture

emma.maria: Yes, for Seven I placed the title block in the header region, which is that high up area.

RainbowArray’s picture

At some point you'd think that Testbot would at least have the courtesy to say something amusing like "You know nothing, Jon Snow," each time I get things terribly wrong.

Trying to change the array key from 'title' to 'page_title' on the chance that 'title' does something special and magical, which seems entirely possible in the wonderful world of render arrays.

I traced back where $main_content['#title'] comes from for at least one of the exceptions, and I'm having a hard time where things are going wrong. Figured out that $main_content['title'] at least typically goes back to a controller setting a response, or sometimes the title is set right in a routing.yml if I am reading things right.

SimplePageVariant gets its $this->title (which is throwing the exception) from HtmlRenderer. The $get_title is getting the title either from $main_content['#title'] or titleResolver. I've traced titleResolver back before, and looking again, that either comes from running the route_title through $this->t (which is definitely a string) or from a title callback. So I suppose it's possible that a title callback somehow returns a non-string, and that's where the exception is coming from? In which case SimplePageVariant would be trying to concatenate the h1 tags with the title value, and if that's an array, I could see that being... exceptional.

So changing title to page_title is probably not going to solve this, but maybe it eliminates some weirdness somewhere.

At least I'm getting a hands-on tour of the render pipeline on this journey. Feeling foolish that I don't know enough to figure this out: if anybody has clues and suggestions, I'm all ears.

RainbowArray’s picture

Looking back to where the exceptions started. Fixing some small discrepancies in the install config file for Stark's page title block.

RainbowArray’s picture

Checked on simplytest.me, and the page title block is being placed in both Bartik and Seven. However the title isn't actually showing up on the page, perhaps not a surprise given all the errors.

I thought I had placed the Seven page title block in the header region, but apparently not. Fixing that now.

Also changing SystemPageTitleBlock back so that its title is a #markup render array. That is the thing that is not showing up on the page. Tracing that back, BlockPageVariant checks to see if a block implements TitleBlockPluginInterface, which this block does. If so, then it uses setTitle to send in the title.

The place where it gets that title is from HtmlRenderer, from that same $get_title() that SimplePageVariant gets its title from.

Ooh! So the Twig template actually gets the title variable from system_preprocess_block, and the page title block case gets that from ['content']['title']['#markup'], which I now realize is coming from the $build step in SystemPageTitleBlock.

This has been episode adventure of "Marc tries to understand Drupal 8." On the next episode, how many shades of yellow and red will Testbot turn after seeing what this patch does!

RainbowArray’s picture

I think what this boils down to is that in HtmlRenderer, $get_title() does not necessarily return a string, either from $main_content['#title'] or from titleResolver (possibly due to callbacks). For some reason, that can end up as an array.

On the other hand, these loads of exceptions didn't show up prior to me adding a page title block for Stark in the minimal profile. I can't think as to why that would cause a problem, but trying to remove that install to see what happens.

RainbowArray’s picture

Well that's interesting. So whatever is going wrong, it has nothing to do with the Stark page title block. That means we should probably base whatever the next patch is off of #52, not #54.

This makes me think that something changed between August 13 and August 31, when the first massive exceptions showed up. Obviously a lot of things changed in core during that time, but something that directly affected this code. Either something in the new code in these patches that isn't meeting new expectations or something went in that's clashing with these changes.

RainbowArray’s picture

Looked at change notices in that time period and git blames for some key files. Nothing new in SimplePageVariant, BlockPageVariant or TitleResolver. There are some new caching things in HtmlRenderer. It doesn't immediately look like that would mess up the way title is working, but it might be someplace to start looking.

RainbowArray’s picture

The issue that affected HtmlRenderer was #2551989: Move replacing of placeholders from HtmlRenderer to HtmlResponseAttachmentsProcessor. It's possible other parts of the SmartCache work had an impact. In general SmartCache is setting up placeholders to be rendered later in the process so that non-cache context sensitive content can be cached, while more context sensitive things aren't cached along with everything else. Since the title is pretty dynamic and can be context sensitive, I'd imagine that would need to not be cached along with other things. So maybe title isn't being flattened as early leaving that an array rather than a string?

A lot of SafeMarkup things went in during this time period too. So that too could in theory have had an impact on how the title variable is handled.

Wim Leers would probably know what's going on with SmartCache, others probably know about SafeMarkup. If anybody has ideas, feel free to share.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
27.02 KB
5.68 KB

Wow, awesome work, @mdrummond, thank you so much for pushing this forward!

I found the root cause for your troubles. #2543340: Convert BlockViewBuilder to use #lazy_builder (but don't yet let context-aware blocks be placeholdered) made block rendering lazy, and special cased the main content block. The title block works in a very similar way (it carries state from the main request), and so we need to special case that one also.

I also found the root cause of the many exceptions: it's a bit tricky, but titles are plain strings 99% of the time, but they can be render arrays for the use cases where we need <em> in the title for example. So with a little bit of "if render array then use it directly, otherwise, map the string to a render array" logic, we can fix all that too.

I didn't run all tests, but this should definitely help reduce the number of test failures.

  1. +++ b/core/modules/shortcut/shortcut.module
    @@ -311,7 +311,7 @@ function shortcut_preprocess_page(&$variables) {
    -      'name' => $variables['title'],
    +      'name' => \Drupal::service('title_resolver')->getTitle(\Drupal::request(), $route_match->getRouteObject()),
    

    This breaks whenever the title originates from the main content itself.

    (See the hunks changed in HtmlRenderer.)

    This is why I think the Shortcut implementation must be rethought.

    I see two options:

    1. Let Shortcut module implement hook_block_view_alter(), to alter the title block. Downside: Shortcut module will only work for pages built using Block module, so e.g. Panels (and other page variants) would have to implement a similar alteration on behalf of Shortcut module.
    2. Introduce a '#type => 'page_title', so hook_element_info_alter() can be implemented by Shortcut module to make the necessary alterations.
  2. +++ b/core/modules/system/src/Plugin/Block/SystemPageTitleBlock.php
    @@ -0,0 +1,79 @@
    +    // The 'Page title' block is never cacheable, because it may be dynamic.
    +    $form['cache']['#disabled'] = TRUE;
    +    $form['cache']['#description'] = t('This block is never cacheable, it is not configurable.');
    +    $form['cache']['max_age']['#value'] = 0;
    ...
    +    // The 'Page title' block is never cacheable, because it may be dynamic.
    +    return FALSE;
    +  }
    

    Hrm, I'm not sure this is still true. Especially with #2429617: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!).

    The title depends on the route (route name + params). Because it is the title for a specific route.

    If we can cache the response for a route, which is the main content (== route controller result) plus the decoration (blocks) per route, then surely we can also cache a response's title for a route? They're deeply connected.

    Fixed.

  3. +++ b/core/modules/system/src/Plugin/Block/SystemPageTitleBlock.php
    @@ -0,0 +1,79 @@
    +  public function isCacheable() {
    

    This is still implementing the old interface. Fixed.

RainbowArray’s picture

Introduce a '#type => 'page_title', so hook_element_info_alter() can be implemented by Shortcut module to make the necessary alterations.

This sounds like the better option to allow Panels, etc to not have to reinvent the wheel.

Thank you so, so, so much for all the fixes Wim!

I would bet that a good chunk of the remaining errors are from tests that need to be updated to 1) enable the block module, and 2) place the page title block in order to see the expected markup. I can definitely work on that, but I'm traveling today. I'll take a look tonight.

RainbowArray’s picture

Status: Needs work » Needs review
FileSize
27.63 KB
619 bytes

Adding back the page title block for Stark in the minimal profile.

RainbowArray’s picture

Here's my attempt to update tests to place the page title block in places where it looks like that's necessary. It's entirely possible I'm wrong on that, but it's an attempt.

One thing is almost definitely wrong is the updates I made to the BlockPageVariantTest. However, I left a llama there in the hopes that Wim might be able to figure it out.

There was also an error on the test related to expected cache contexts. It said it didn't expect route, so I added that to the list of cache contexts that test was expecting. But it's entirely possible that is wrong.

I'm not sure how to fix the shortcut module.

I did check and titles are actually showing up again, which is definitely a step in the right direction!

Wim Leers’s picture

Assigned: Unassigned » Wim Leers

I'm working on fixing the remaining test failures now. Making good progress.

Wim Leers’s picture

Assigned: Wim Leers » Unassigned
Status: Needs work » Needs review
FileSize
46.28 KB
7.9 KB

However, I left a llama there in the hopes that Wim might be able to figure it out.

:D You trick me!


This fixes all test failures that are unrelated to:

  • cache context stuff
  • title prefix/suffix stuff

The solution for both of those is to have a '#type' => 'page_title'. Will do that in the next reroll.

Wim Leers’s picture

This introduces '#type' => 'page_title', and fixes the cache context-related failures.

(Accidentally unassigned, now reassigning.)

Next: title prefix/suffix stuff, on which I might get stuck. Let's see.

Wim Leers’s picture

I forgot to include one crucial file in #68… Therefore #68 will fail *very* hard :P

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
65.32 KB
15.57 KB

Ah, great — as expected! The route cache context is now being added for every page title, and is thus causing quite a few test failures, because the expectations of many tests are outdated.

Wim Leers’s picture

Since I introduced '#type' => 'page_title', empty titles (i.e. no titles) weren't handled correctly.

Wim Leers’s picture

'#type' => 'page_title''s #pre_render callback generates #markup but the ensureMarkupIsSafe() call happens before that, hence we lose that safety guarantee. This took a while to figure out.

This should be reviewed by Alex Pott.

Wim Leers’s picture

Considerations to solve The Shortcut Problem:

  1. Make it a block. From a markup POV this is perfectly doable. However, there is no way to get the title, except by using JS, which we don't want to.
  2. So we must continue to alter the existing "page title" thing. But it turns out that title_suffix can only be set in theme preprocess functions, not in render arrays (but perhaps I'm wrong!). So… that leaves only one choice: change shortcut_preprocess_page() to shortcut_preprocess_block__system_page_title_block(). This means the handy way to create a shortcut (the shortcut shortcut I guess…) is only available when you use the page title block. Which could be acceptable.
  3. Or, alternatively, move the bulk of the page title block template into a page title template, so that we can have shortcut_preprocess_page_title() and page-title.html.twig.

For now, I went with option 2. But somehow, I'm getting the title suffix printed twice. I can't figure out why. Since I'm not even sure option 2 is the best choice, plus the fact that I don't know how this title-suffix-only-in-theme-layer thing is supposed to work exactly, I'm leaving that to you, @mdrummond.

I think the only other thing that still needs to be fixed is Views' contextual links. That's using all sorts of crazy hacks to work, and apparently we are breaking those.

Wim Leers’s picture

+++ b/core/lib/Drupal/Core/Render/Element/PageTitle.php
@@ -0,0 +1,61 @@
+      $element['#markup'] = $element['#title'];

I think changing #markup to #plain_text would fix PageTitleTest.

And that makes sense too: the behavior in HEAD is that titles are escaped by default, unless they specify a render array with #markup and #allowed_tags.

We probably want to add an assert() call to ensure #title indeed is either a plain string or a render array with those two keys.

Frando’s picture

Is it save to assume a page title only depends on the 'route' cache context? Couldn't I do something like this in a controller:

$build['#title'] = $this->t('Hi there, !user ', array('!user' => $currentUser->get('name'));

or even

$query = \Drupal::entityQuery('node')->condition('status', 1)->count();
$count = $query->execute();
$build['#title'] = $this->t('Welcome to a dynamic site with !count published posts', array('!count' => $count);

Not sure, but wouldn't this break with smartcache then? I didn't test yet, but maybe the title block should inherit the main content's cacheability metadata.

Wim Leers’s picture

#82: You're absolutely right. It will break even without SmartCache, just render caching of the block will already be problematic.

The solution is simple in principle: if you have such a dynamic title, you must not do:

return t('Hi there, !user', …);

but:

return [
  '#markup' => t('Hi there, !user', …);
  '#cache' => [
    'contexts' => [
      'user',
    ],
  ],
];

But this means that also all _title_callbacks have to be evaluated, to check if they do dynamic stuff like this. Technically, this cacheability metadata should've been specified all along…

RainbowArray’s picture

Reroll of #76 due to changes in DisplayBlockTest.

RainbowArray’s picture

This is an attempt to address the concerns with PageTitle, although I wasn't sure what to do with the assert request. Definitely need eyes on this to see if this is right.

My understanding is that for #markup there are certain tags that do not need to go into #allowed_tags, such as h1. So I'm not certain that #allowed_tags is required here.

In #76, option 3 sounds in theory good. Having a dedicated page title template is not out of line with some of the other block conversions we've done. I think in theory that might also allow us to skip putting the h1 tags in #markup, which would be good. SimplePageVariant might end up being used rarely for actual front-end work, but changing that h1 would be a pain if you needed to do so . So +1 to template.

lauriii’s picture

+++ b/core/lib/Drupal/Core/Render/Element/PageTitle.php
@@ -53,7 +54,7 @@ public static function preRenderPageTitle($element) {
+      $element['#plain_text'] = $element['#title'];

I was playing with this but there is a problem that in some use cases $element['#title'] has already rendered markup inside it which doesn't work well with the #plain_text :P I think we have to figure out where this markup is coming from

lauriii’s picture

Status: Needs work » Needs review
FileSize
66.5 KB

reroll

DuaelFr’s picture

Take care, using TitleResolver can cause fatals on Views pages.
See #2516742: Allow Views to be resolved by TitleResolver

lauriii’s picture

Status: Needs work » Needs review
FileSize
961 bytes
66.33 KB

The title is already rendered in some cases when it comes to the PageTitle render element so closest we can get there is to use #markup. Why wouldn't we allow other types of render arrays than #markup? I removed that limitation and it seemed to fix the title at least for some of the pages :)

lauriii’s picture

Status: Needs work » Needs review
FileSize
72.52 KB

Reroll with a bit of a fuzz.

lauriii’s picture

Assigned: Unassigned » lauriii

I have some work in progress to fix this. Will be posting about my progress later today

tim.plunkett’s picture

  1. +++ b/core/lib/Drupal/Core/Block/MainContentBlockPluginInterface.php
    @@ -10,7 +10,7 @@
    - * A main page content block represents the content returns by the controller.
    + * A main page content block represents the content returned by the controller.
    

    Not really in scope, this file is untouched otherwise.

  2. +++ b/core/modules/block/src/Plugin/DisplayVariant/BlockPageVariant.php
    @@ -131,6 +149,10 @@ public function build() {
    +        elseif ($block_plugin instanceof TitleBlockPluginInterface) {
    +          $block_plugin->setTitle($this->title);
    +          $title_block_displayed = TRUE;
    +        }
    

    Yet Another Special Cased Block That Contrib Cannot Mimic Because It Would Require Hacking Core

  3. +++ b/core/modules/block/tests/src/Unit/Plugin/DisplayVariant/BlockPageVariantTest.php
    @@ -176,7 +183,7 @@ public function providerBuild() {
    +        // The main content, messages and title are rendered via the fallback in case
    

    main content, messages, and title

Wim Leers’s picture

Yet Another Special Cased Block That Contrib Cannot Mimic Because It Would Require Hacking Core

No, contrib can implement that interface too. So contrib can mimic it.

lauriii’s picture

Assigned: lauriii » Unassigned

We might need to get #2359901: Discourage $main_content['#title'] in favor of route titles and title callbacks before this can properly work because otherwise we have to early render the main content which causes problems with the PageTitle render element. Other option is to keep extending the hack that we have there..

Wim Leers’s picture

No, we can't have #2359901: Discourage $main_content['#title'] in favor of route titles and title callbacks, it already seemed unlikely when I opened it, but this close before RC we can't do that anymore.

If HEAD can work with $main_content['#title'], then this patch should be able to as well.

I can take care of that title escaping/already rendered weirdness, if you can handle the shortcut stuff. See #76.


The bigger threat to this issue IMHO is #82 + #83. We may want sign-off on that first. I'll bring it up during the D8 EU criticals call later today.

RainbowArray’s picture

This creates a template for the page title which is used instead of the page title block template, which isn't necessary unless we want to remove the default block wrapper markup. Or at least this is my best guess as to how to do that. I may very well be wrong.

This then lets shortcut use preprocess_page_title. If this works, I like being able to always have a template for the page title even when there are no blocks. That is a lot cleaner.

RainbowArray’s picture

theme.inc probably needs to set template for page title render element like status messages does. Maybe. That's my last thought before slumberzzzzzzzz

dawehner’s picture

We talked about that on the D8 critical meeting and came to the following conclusion:

  • Breadcrumbs aren't a problem because they can just vary on url/route, everything else is not provided when they are rendered
  • For #title you cannot use a placeholder approach, because you need to execute the main controller to be able to determine the content of #title. Views for example has that advanced usecase. The suggestion is to copy over the cachability metadata from the main content to the title block. This will result in no lost cacheability at the end, so no performance issue.
RainbowArray’s picture

I think I was wrong about specifying the template for page_title. But this is still pretty broken. Definitely could use eyes from other theme team people on this.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
65.63 KB
2.05 KB

Removed 'render element' as it's not needed and makes things unnecessarily trickier for preprocess.

And the twig {% extend @block %} left in the template wasn't needed and breaks things.

RainbowArray’s picture

I think this should help with any markup that might be in the title like em tags. Also fixing unnecessary prefixes and suffixes with h1 tags in a test.

I did manually check this, and page titles are showing up, which is good. I can add a shortcut from the title, so at least something is working there.

I'm wondering if maybe we should use a block template in order to remove the block wrapper markup around the title. That might not be necessary, as it looks like page titles look the same in Bartik and Seven. In Seven, the block contextual links go behind the main content area. That wasn't an issue before because there was no contextual links for the title in Seven. We might need a CSS fix for that.

Quick Edit sort of works. It appears that it's working, but after you save a change and reload the page, the change doesn't save.

cilefen’s picture

In #116, the page title block is positioned below the page content on the freshly-installed home page with, the one with "No front page content has been created yet."

RainbowArray’s picture

#118. Hmm. I'm not seeing that, at least not with Bartik.

RainbowArray’s picture

#118: I'm not seeing that with a fresh install. We don't have an upgrade path yet, so if you're doing this with an existing site that might be why.

RainbowArray’s picture

With #116, the span around title for QuickEdit is showing up in quotation marks. Because reasons.

Wim Leers’s picture

Assigned: Unassigned » Wim Leers
Status: Needs work » Needs review
FileSize
64.83 KB
1.61 KB

Trying to get this to green today. Assigning to myself.

+++ b/core/lib/Drupal/Core/Render/Element/PageTitle.php
@@ -50,12 +51,6 @@ public static function preRenderPageTitle($element) {
-    if (is_array($element['#title'])) {
-      $element += $element['#title'];
-    }
-    else {
-      $element['#markup'] = $element['#title'];
-    }

@joelpittet removed this in #113, and I thought "WTF".

@mdrummond added it back in #116 and I thought "yay".

But @joelpittet's change there was genius: this just makes it so that we bring back the exact same behavior as in HEAD. We have a title (either a string or a render array) that we pass to Twig, and let Twig handle it (and possibly render it). That's exactly what we were looking for in the last few comments/days! :)

So, bringing back that change, and removing even more that we then don't need anymore.

@joelpittet++
@joelpittet++
@joelpittet++

Wim Leers’s picture

To implement #111, which addresses #82 + #83, I see roughly four options:

  1. Always pass the page title as a render array, so we can pass cacheability metadata. This has lots of tricky consequences though, because that means I can no longer pass a simple string around as a title, it needs to be either #markup or #plain_text, and it needs to match TwigExtension::escapeFilter()'s behavior, so that plain strings are escaped, and safe strings aren't.
  2. Add a PageTitleSafeString safe string implementation, which also carries cacheability metadata. But, this means that if the title is a plain string, we must already do the escaping that Twig would otherwise do, which means we can no longer access the unescaped title that we need for <title>. So this does not actually work.
  3. Always pass the page title not just as the render array returned by a controller or title callback, but as a #type => page_title render array. This just requires some interfaces to be updated to not accept the page title as a string, but only as a #type => page_title render array. Then we don't run into the "how to handle the different types of strings and mimic Twig" problem in option 1.
  4. Expand \Drupal\Core\Block\TitleBlockPluginInterface so it can receive the cacheability metadata of the main content block via a new method. This feels terrible. And still would only work for blocks; the solution must work for #type => page_title, so that it also works for alternative page construction mechanisms (like Page Manager, Panels …).

I implemented both options 1 (solution-a-interdiff.txt) and 2 (solution-b-interdiff.txt).

… and then after I'd done all this (few hours of) work, I realized there's a much more logical, and much more simple solution!

We're doing all this work so that the title block can be cached correctly. But, there is no value in caching this block! And caching the title block by all the cache contexts that the main content varies by, just means that the title block's cache redirect would eventually end up containing every possible cache context, which means that even the title for a simple route controller's main content, that doesn't vary by anything, still would need to vary by every possible cache context, i.e. the cache hit ratio would be horrendous.
That's a lot of complexity and overhead for actually even worse performance.

And the simple answer is: it does not make sense to cache the title block, just like it does not make sense to cache the main content block! Both are always calculated anyway by the route controller. So we should simply special case the title block just like we special case the main content block. That also makes it so that everything continues to work exactly like it does today, in terms of things being calculated/rendered. This patch then just allows you to move the title around thanks to it being a block.

Status: Needs review » Needs work

The last submitted patch, 126: convert_page_title_to_block-2476947-126.patch, failed testing.

The last submitted patch, 126: convert_page_title_to_block-2476947-126.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
66.68 KB
1.84 KB
  1. The last cache context fix: #type => page_title already is specifying the route cache context; the block doesn't need to do the same
  2. RolesRidArgumentTest had a very strange assertion, that I suspect was asserting a bug: it was asserting that if an entity has a label with <em>, that that <em> was output in escaped form. This patch fixes that. I expanded the assertion, to ensure that it is not just passing the HTML without any filtering.
Wim Leers’s picture

The way the HTML's <title> tag is generated is very, very complex and forces us to duplicate a bit of logic in shortcut_preprocess_page_title(). But the only alternative is refactoring the way that <title> is computed, but that's very much out of scope for this issue; it's a problem that has existed for many years, layers of legacy over older layers of legacy. So I think this small bit of duplication is the better solution for now.

This fixes the shortcut failures. If all is well, the only failures left now, should be the Views contextual links ones.

EDIT: Note that the second hunk here is a change made because ShortcutLinksTest::testNoShortcutLink() specifically asserts that the shortcut link appears even when there is no title.

The last submitted patch, 129: convert_page_title_to_block-2476947-129.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 130: convert_page_title_to_block-2476947-130.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
66.92 KB
930 bytes

Oops, I got the order/if-test of the first hunk in #130 wrong.

Wim Leers’s picture

+++ b/core/modules/system/templates/page-title.html.twig
+++ b/core/themes/bartik/templates/page-title.html.twig
+++ b/core/themes/seven/templates/page-title.html.twig

There's far too much repetition in these templates, let's put Twig's power to good use to avoid that :)

The last submitted patch, 129: convert_page_title_to_block-2476947-129.patch, failed testing.

The last submitted patch, 130: convert_page_title_to_block-2476947-130.patch, failed testing.

davidhernandez’s picture

I approve of extending, but there should definitely be a Classy version of the page-title template, and have Seven and Bartik extend off of that. I'm thinking it would go in the "content" folder.

The last submitted patch, 133: convert_page_title_to_block-2476947-133.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 134: convert_page_title_to_block-2476947-134.patch, failed testing.

Wim Leers’s picture

So, Classy would add page-title, since that's what both Bartik and Seven use? And then Bartik would extend Classy's, and just add title?

davidhernandez’s picture

Yeah, copy the system template to Classy and just extend Seven and Bartik off of that one. We have multiple reasons for having templates in Classy, not just if they are different from system. And if Bartik and Seven are going to extend something it would be from Classy.

The last submitted patch, 133: convert_page_title_to_block-2476947-133.patch, failed testing.

The last submitted patch, 134: convert_page_title_to_block-2476947-134.patch, failed testing.

davidhernandez’s picture

The interdiff looks a little funny because it thinks I moved the Seven template.

Also, this made me so happy.

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Block/TitleBlockPluginInterface.php
    @@ -0,0 +1,29 @@
    +   *
    +   * @param string $title
    +   *   The page title.
    +   */
    +  public function setTitle($title);
    
    +++ b/core/lib/Drupal/Core/Render/Plugin/DisplayVariant/SimplePageVariant.php
    @@ -28,6 +28,13 @@ class SimplePageVariant extends VariantBase implements PageVariantInterface {
    +   * The page title.
    +   *
    +   * @var string
    +   */
    +  protected $title = '';
    
    @@ -38,9 +45,22 @@ public function setMainContent(array $main_content) {
        * {@inheritdoc}
        */
    +  public function setTitle($title) {
    +    $this->title = $title;
    +    return $this;
    +  }
    

    I've seen the use of #title => ['#markup'] so is this doumentation really the right one?

  2. +++ b/core/modules/shortcut/shortcut.module
    @@ -309,9 +309,17 @@ function shortcut_preprocess_page(&$variables) {
    +    // Replicate template_preprocess_html()'s processing to get the title in
    +    // string form, so we can set the default name for the shortcut.
    +    if (is_array($variables['title'])) {
    +      $name = (string) \Drupal::service('renderer')->render($variables['title']);
    +    }
    +    else {
    +      $name = $variables['title'];
    +    }
    

    I'm curious whether this should call the old render() function which had that kind of logic

  3. +++ b/core/modules/shortcut/src/Tests/ShortcutTranslationUITest.php
    @@ -21,7 +21,7 @@ class ShortcutTranslationUITest extends ContentTranslationUITestBase {
    diff --git a/core/modules/system/src/Plugin/Block/SystemPageTitleBlock.php b/core/modules/system/src/Plugin/Block/SystemPageTitleBlock.php
    
    diff --git a/core/modules/system/src/Plugin/Block/SystemPageTitleBlock.php b/core/modules/system/src/Plugin/Block/SystemPageTitleBlock.php
    new file mode 100644
    
    new file mode 100644
    index 0000000..5615701
    
    index 0000000..5615701
    --- /dev/null
    
    --- /dev/null
    +++ b/core/modules/system/src/Plugin/Block/SystemPageTitleBlock.php
    
    +++ b/core/modules/system/src/Plugin/Block/SystemPageTitleBlock.php
    +++ b/core/modules/system/src/Plugin/Block/SystemPageTitleBlock.php
    @@ -0,0 +1,63 @@
    

    Does someone mind moving this to lib/Drupal/Core/Render/Block ... much like we did with the local tasks/actions blocks?

  4. +++ b/core/modules/system/src/Plugin/Block/SystemPageTitleBlock.php
    @@ -0,0 +1,63 @@
    +   */
    +  public function defaultConfiguration() {
    +    return ['label_display' => FALSE];
    +  }
    

    Is that a c&p? I don't see the option used

  5. +++ b/core/modules/system/templates/page-title.html.twig
    @@ -0,0 +1,22 @@
    + * Available variables:
    + * - title_prefix: Additional output populated by modules, intended to be
    + *   displayed in front of the main title tag that appears in the template.
    + * - title: The page title, for use in the actual content.
    + * - title_suffix: Additional output populated by modules, intended to be
    + *   displayed after the main title tag that appears in the template.
    ...
    +  <h1{{ title_attributes }}>{{ title }}</h1>
    

    Should we document title_attributes as well?

Status: Needs review » Needs work

The last submitted patch, 144: convert_title_page-2476947-144.patch, failed testing.

Wim Leers’s picture

Assigned: Wim Leers » Unassigned
Status: Needs work » Needs review
FileSize
72.66 KB
6.51 KB

Have to run. Did not run tests. But it does work in the UI. Perhaps the tests need to be adjusted.

All the INSANE work-around of Views plus contextual links is now GONE as a necessary requirement to get this patch to green, hurray! :)

18:49:25 <davidhernandez> WimLeers: not sure why those context link asserts are failing. the ids look correct.
18:50:33 <WimLeers> davidhernandez: because views_add_contextual_links() and views_preprocess_html() and all the craziness that Views has
18:50:42 ⇐ mherchel quit (~mherchel@ip68-101-64-98.ga.at.cox.net) Remote host closed the connection
18:50:50 <WimLeers> davidhernandez: I've got an idea, but it's going to take some more time to get to work, and I need to go AFK in a few minutes
18:50:57 <WimLeers> davidhernandez: the idea is quite simple
18:51:03 <WimLeers> davidhernandez: eradicate all this craziness
18:51:17 <WimLeers>   // If the page contains a view as its main content, contextual links may have
18:51:17 <WimLeers>   // been attached to the page as a whole; for example, by
18:51:17 <WimLeers>   // views_page_display_pre_render().
18:51:17 <WimLeers>   // This allows them to be associated with the page and rendered by default
18:51:17 <WimLeers>   // next to the page title (which we want). However, it also causes the
18:51:17 <WimLeers>   // Contextual Links module to treat the wrapper for the entire page (i.e.,
18:51:17 <WimLeers>   // the <body> tag) as the HTML element that these contextual links are
18:51:24 <WimLeers> That and everything else there no longer makes sense!
18:51:29 <WimLeers> The view is the main content.
18:51:30 <WimLeers> Period.
18:51:35 <WimLeers> It's NOT the entire page
18:51:51 <WimLeers> So my plan is to:
18:52:00 ⇐ emmamaria quit (~Emma@212-51-157-186.fiber7.init7.net) Quit: emmamaria
18:52:02 <WimLeers> 1) remove system_block_view_system_main_block_alter(), so that the main content block CAN again have contextual links
18:52:26 <WimLeers> 2) then make sure that views with a Page display actually have their contextual links appear on the main content block
18:52:36 <WimLeers> If you do just 1) you can already see how it's going to work
18:53:28 <davidhernandez> so what contextual links are missing? i see them on the node view
18:54:12 <WimLeers> davidhernandez: the "Edit view" contextual link
18:54:43 <davidhernandez> oh ic
18:55:29 <WimLeers> davidhernandez: I've dealt with this (s/dealt/fought/) in the past
18:55:35 <WimLeers> It is absolutely mind-bogglingly crazy
18:56:19 ⇐ kgoel quit (~kgoel@pool-108-51-33-181.washdc.fios.verizon.net) Remote host closed the connection
18:57:10 <WimLeers> davidhernandez: lol I just got it working half by accident

The last submitted patch, 144: convert_title_page-2476947-144.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 147: convert_page_title_to_block-2476947-145.patch, failed testing.

The last submitted patch, 147: convert_page_title_to_block-2476947-145.patch, failed testing.

davidhernandez’s picture

patch applied, not sure why it failed to run.

davidhernandez’s picture

manually testing 147, can confirm the contextual links are back.

The last submitted patch, 147: convert_page_title_to_block-2476947-145.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
73.52 KB
1007 bytes

Fixed the fatal.

Wim Leers’s picture

Contextual links tests now pass.

The last submitted patch, 155: convert_page_title_to_block-2476947-155.patch, failed testing.

The last submitted patch, 155: convert_page_title_to_block-2476947-155.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 156: convert_page_title_to_block-2476947-156.patch, failed testing.

The last submitted patch, 156: convert_page_title_to_block-2476947-156.patch, failed testing.

davidhernandez’s picture

Assigned: Unassigned » davidhernandez

Ok, I think I can fix that now.

davidhernandez’s picture

Assigned: davidhernandez » Unassigned
Status: Needs work » Needs review
FileSize
75.68 KB
663 bytes

Hopefully, this one's a winner.

RainbowArray’s picture

YAYYYYYYY!!!!

Now for last fixes!

#145
- 1. I don't know how to document something that can be either a string or a render array.
- 2. I don't know.
- 3. I moved this to Block as requested. For local actions we moved it to Menu, which was slightly different and perhaps a bit more logical place to look. But hopefully this still works? I like page_title_block a lot more than system_page_title_block as an ID. Maybe we should do a follow-up to move the other system page element blocks. Although I don't know what that upgrade path would look like.
- 4. I believe this is setting the block to not show the label for the block by default, which should be fine to do here. label_display is a default config option for all blocks.
- 5. Sure, documented title_attributes as well.

Next we'll need an upgrade path.

RainbowArray’s picture

This adds an upgrade path for the page title.

One important note: I added a page title block to Classy for the standard profile. Right now there are no blocks enabled by default in the standard profile for Classy, but we should really have a follow-up to enable all the same blocks in Classy as are enabled in Stark for the standard profile. Classy is essentially what Stark used to be (with increasingly improved markup) , and enabling blocks for the standard profile shouldn't mess up tests, which as far as I know uses the testing profile in the cases where Classy is being tested by default.

I also added the page title block to Classy's content region in the upgrade path. That will help mitigate any issues with "Dude, where'd my page title go?" Although technically if a page title block isn't placed, it still shows up in the main content area, which works even better now that we have a page title template.

We still need to address 1 and 2 from #145. And we should get some good code reviews. But hopefully we are very close to RTBC and getting this in there. Thanks David and Wim for doing the heavy lifting of fixing all those tricksy tests!

RainbowArray’s picture

Just to double-check, re-uploading these patches as I had missed doing a commit before doing the patch and interdiff.

The last submitted patch, 164: 2476947-164-convert-page-title-block.patch, failed testing.

Wim Leers’s picture

Addressed the remaining feedback:

  • #145.1: I'm glad you remarked that, I thought the same. Fixed!
  • #145.2: hah, clever!

Also fixed indentation nits I found in #163–#165.


Next: do a complete review of my own, and try to revert some of the changes along the way that turn out to be unnecessary in the end.

Wim Leers’s picture

I believe #75 could be inverted completely. It's not necessary here, and hence out of scope. I moved that interdiff into a separate issue, because it's an independent problem: #2567715: #markup generated by #pre_render callbacks do not get processed by ensureMarkupIsSafe().

Slightly simpler patch!

Wim Leers’s picture

FileSize
1.45 KB

#168 contained the same interdiff as #167 and was thus clearly wrong, here's the right one. (It's the inverse of #75.)

Wim Leers’s picture

We're placing the page title block in many tests now. But the whole point of the fallback is that … there is a fallback that works. So, reverting all of those.

Wim Leers’s picture

As of #137/#140/#141, the Classy template now has the page-title class on the page title's <h1>. This means we can target the page title using that instead of the relatively hacky //main//h1 XPath expression that I used.

Wim Leers’s picture

Before #126, the title block was being cached. So we need to ensure the right cacheability metadata was present. As part of that, we ensured that the route cache context was set on the title. This caused a whole avalanche of test changes.

However, just like the main content originates from the route's controller, so does the title originate from the route controller, route's static title (_title) or dynamic title (_title_callback). But we also don't set the route cache context on the main content. Arguably we should do that, but for now it's better for the title to be consistent with the main content. Because we now treat the route's title the same way as the route's main content, we should also treat their cacheability the same: neither are cached, and neither have hardcoded cache contexts set.

This makes the patch a whole lot smaller. :)

Wim Leers’s picture

The only remaining thing I wanted to do is to make Views' contextual links work even without blocks — as of #147, a lot of the complexity in Views wrt contextual links for its Page display are gone, with only one negative side effect: Views' Page displays now only have contextual links if the "main content" block is being used.

Even though that only affects a minority of sites, I think that's the main thing left that people can complain about in this patch.

Together and yesterday combined I've now spent >6 hours on figuring out a solution to this problem. Sadly, there are many, many, many layers of rabbitholes with Views contextual links handling for Page displays. It actually dates back to the initial Views commit to Drupal 8 core (commit a626abb24faa51ac140f73779a89e1ad7d5ae716), which included the hacks necessary for Drupal 7 (grep for view_array), and since then, the hacks have only grown more complex, mysterious and brittle (#1849822: Convert (HTML) view rendering to a render array, #2381277: Make Views use render caching and remove Views' own "output caching"). For example: we still have view_array, which itself was a hack around limitations in Drupal 7's theme system, added solely for the purpose of supporting contextual links, but then we don't ever populate it anymore in Drupal 8.

Therefore, I propose we go forward with this patch without blocking it on resolving all of the crazy work-arounds for Views + Contextual links built up over the past >6 years.

Wim Leers’s picture

Having written #173, and having explored several approaches to fix it, I finally saw a solution! When using the fallback, the main content should get a theme wrapper that prints title_suffix, and that's all we really need!

So, with this approach, you'll get contextual links for Page views even when not using the "main content" block.

Comes with expanded test coverage.

joelpittet’s picture

+++ b/core/modules/system/templates/container--main-content.html.twig
@@ -0,0 +1,12 @@
+<div {{ attributes }}>{{ title_suffix }}{{ children }}</div>

Minor nit: extra space in there before attributes, not needed.

I'll do a bit of a thourough review later.

Status: Needs review » Needs work

The last submitted patch, 174: convert_page_title_to_block-2476947-174.patch, failed testing.

The last submitted patch, 174: convert_page_title_to_block-2476947-174.patch, failed testing.

The last submitted patch, 174: convert_page_title_to_block-2476947-174.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
61.37 KB
2.99 KB

#175: fixed.

Fixed the two fails.

The CommentPagerTest was a WTF, especially because the test coverage added in #2346119: Fix call to undefined method Select::setCountQuery() did not make any sense. But that wasn't even the root cause, the root cause is that \Drupal\comment\Tests\CommentTestBase::commentExists() is relying on regexps instead of actually traversing the DOM tree. The changes here trigger a false positive, and hence a test failure. No more regexp now.

joelpittet’s picture

+++ b/core/modules/comment/src/Tests/CommentTestBase.php
@@ -188,14 +188,7 @@ public function postComment($entity, $comment, $subject = '', $contact = NULL, $
-      $regex = '!' . ($reply ? '<div class="indented">(.*?)' : '');
-      $regex .= '<a id="comment-' . $comment->id() . '"(.*?)';
-      $regex .= $comment->getSubject() . '(.*?)';
-      $regex .= $comment->comment_body->value . '(.*?)';
-      $regex .= ($reply ? '</article>\s</div>(.*?)' : '');
-      $regex .= '!s';
...
+      return (bool) $this->cssSelect('.comment-wrapper ' . ($reply ? '.indented ' : '') . '#comment-' . $comment->id());

This seems like a great change but should it not assert the subject and body exist as it did before, do you think? Right now it's testing the structure but not the content, before it was doing both, although poorly.

You could check inside the match maybe for those two?

lauriii’s picture

  1. +++ b/core/includes/theme.inc
    @@ -1290,9 +1290,6 @@ function template_preprocess_html(&$variables) {
    -  // Move some variables to the top level for themer convenience and template cleanliness.
    -  $variables['title'] = $variables['page']['#title'];
    -
    

    Yay!!

  2. +++ b/core/lib/Drupal/Core/Block/Plugin/Block/PageTitleBlock.php
    @@ -0,0 +1,56 @@
    +use Drupal\Core\Form\FormStateInterface;
    

    Unused

  3. +++ b/core/lib/Drupal/Core/Block/TitleBlockPluginInterface.php
    @@ -0,0 +1,30 @@
    +   *   The page title: either a string for plain titles or a #markup render
    +   *   array for formatted titles.
    

    Is there specific reason why only render arrays with #markup are supported?

  4. +++ b/core/modules/block/src/BlockViewBuilder.php
    @@ -104,7 +105,7 @@ public function viewMultiple(array $entities = array(), $view_mode = 'full', $la
    -      if ($plugin instanceof MainContentBlockPluginInterface) {
    +      if ($plugin instanceof MainContentBlockPluginInterface || $plugin instanceof TitleBlockPluginInterface) {
    

    Might be out of scope but there might be use cases where someone could use a more general interface to opt-out block from being loaded lazily.

  5. +++ b/core/modules/block/tests/src/Unit/Plugin/DisplayVariant/BlockPageVariantTest.php
    @@ -176,14 +183,22 @@ public function providerBuild() {
    +        // The main content, messages and title are rendered via the fallback in case
             // there are no blocks rendering them.
    

    Nit: over 80 chars

  6. +++ b/core/modules/block/tests/src/Unit/Plugin/DisplayVariant/BlockPageVariantTest.php
    @@ -200,11 +215,13 @@ public function providerBuild() {
    +    $display_variant->setTitle('Hi cats!');
    

    Yay, cats!

  7. +++ b/core/modules/system/src/Tests/Update/PageTitleConvertedIntoBlockUpdateTest.php
    @@ -0,0 +1,71 @@
    +    // Page title is visible on the home page.
    +    $this->drupalGet('/node');
    +    $this->assertRaw('page-title');
    

    Should be quick to add test also for a node page. Just in case they work a little bit differently.

  8. +++ b/core/modules/system/system.install
    @@ -1642,5 +1642,86 @@ function system_update_8007() {
    +        break;
    +      case 'classy':
    ...
    +        break;
    +      default:
    

    Ubernit: Whitespace between break and new case :)

  9. +++ b/core/modules/views/tests/src/Unit/Routing/ViewPageControllerTest.php
    @@ -181,3 +181,13 @@ public function testHandleWithArgumentsOnOverriddenRouteWithUpcasting() {
    +namespace {
    +  // @todo replace views_add_contextual_links()
    +  if (!function_exists('views_add_contextual_links')) {
    +    function views_add_contextual_links() {
    +    }
    +  }
    +}
    

    Hmm?

  10. +++ b/core/themes/bartik/templates/page-title.html.twig
    @@ -0,0 +1,18 @@
    + * @ingroup themeable
    

    Should not be in the overriding template

  11. +++ b/core/themes/classy/templates/content/page-title.html.twig
    @@ -0,0 +1,23 @@
    + * @ingroup themeable
    

    I guess it might be same here

dawehner’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/Render/Element/PageTitle.php
    @@ -0,0 +1,31 @@
    +/**
    + * Provides a render element for the title of an HTML page.
    + *
    + * This represents the title of the HTML page's body.
    + *
    + * @RenderElement("page_title")
    + */
    +class PageTitle extends RenderElement {
    

    Does it really make sense to have a page_title render element? Maybe I just don't get it. I would be happy about an answer, maybe this helps me to understand the point of render elements better

  2. +++ b/core/modules/system/src/Tests/Update/PageTitleConvertedIntoBlockUpdateTest.php
    @@ -0,0 +1,71 @@
    +    /** @var \Drupal\Core\Extension\ThemeHandlerInterface $theme_handler */
    +    $theme_handler = \Drupal::service('theme_handler');
    +    $theme_handler->refreshInfo();
    

    Uh, we need a line explaining why we need this ... this is potential dangerous, given that it could hide issues on the update path. I know why, because Extension objects has the root stored, but I thought we fixed that already? We maybe need to apply the same fix to the ActiveTheme object

  3. +++ b/core/modules/system/system.install
    @@ -1642,5 +1642,86 @@ function system_update_8007() {
    +        $name = sprintf('block.block.%s_page_title', $theme_name);
    ...
    +          'id' => sprintf('%s_page_title', $theme_name),
    

    Seriously, this just makes it harder to read compared to just use "block.block.{$theme_name}_page_title"

  4. +++ b/core/modules/system/templates/container--main-content.html.twig
    @@ -0,0 +1,12 @@
    +<div {{ attributes }}>{{ title_suffix }}{{ children }}</div>
    

    The space is not removed yet

+++ b/core/modules/system/system.install
@@ -1642,5 +1642,86 @@ function system_update_8007() {
+      case 'bartik':
+        $name = 'block.block.bartik_page_title';
+        $values = [
+          'id' => 'bartik_page_title',
+        ] + $page_title_default_settings;
+        _system_update_create_block($name, $theme_name, $values);
+        break;
...
+        $name = 'block.block.stark_page_title';
+        $values = [
+          'id' => 'stark_branding',
+          'region' => 'content',
+        ] + $page_title_default_settings;
+        _system_update_create_block($name, $theme_name, $values);
+        break;
...
+        $name = 'block.block.classy_page_title';
+        $values = [
+          'id' => 'classy_page_title',
+          'region' => 'content',
+        ] + $page_title_default_settings;
+        _system_update_create_block($name, $theme_name, $values);
+        break;
...
+        $values = [
+          'id' => sprintf('%s_page_title', $theme_name),
+          'region' => 'content',
+          'weight' => '-50',
+        ] + $page_title_default_settings;

Here is an additional question: If we look at the current page template, the title is above the highlighted region, but here we set the block to the content region, not the highlighted reason, so are we sure this update is the right thing?

Wim Leers’s picture

#183.1: It exists so that shortcut module and others have a way of adding prefixes and suffixes to the page title, because it can consistently target the page title, regardless of how it is rendered: in a block, via Panels …

davidhernandez’s picture

If we look at the current page template, the title is above the highlighted region...

Where do you see that? Title prints right above page.content.

...
{{ page.highlighted }}

  {{ page.help }}

  <main role="main" class="js-quickedit-main-content">
    <a id="main-content" tabindex="-1"></a>{# link is in html.html.twig #}

    <div class="layout-content">

      {{ title_prefix }}
      {% if title %}
        <h1>{{ title }}</h1>
      {% endif %}
      {{ title_suffix }}
      {{ page.content }}
    </div>{# /.layout-content #}
...
dawehner’s picture

#183.1: It exists so that shortcut module and others have a way of adding prefixes and suffixes to the page title, because it can consistently target the page title, regardless of how it is rendered: in a block, via Panels …

Well, right, they are using a preprocess function which is a theme level thing, isn't it, not a render level thing.

Ah I see, in the patch there are more special cases than normal cases.

--- a/core/themes/bartik/templates/maintenance-page.html.twig
+++ b/core/themes/bartik/templates/maintenance-page.html.twig
@@ -35,9 +35,6 @@



- {% if title %}
-
{{ title }}

- {% endif %}
{{ page.content }}
{{ page.highlighted }}

--- a/core/themes/classy/templates/layout/maintenance-page.html.twig
+++ b/core/themes/classy/templates/layout/maintenance-page.html.twig
@@ -35,10 +35,6 @@


- {% if title %}
-
{{ title }}

- {% endif %}
-
{{ page.highlighted }}

{{ page.content }}

davidhernandez’s picture

I see, maintenance page ... sigh

dawehner’s picture

I see, maintenance page ... sigh

Well, some manual testing for those pages would be nice.

Did also anyone did some benchmarking for the case without dynamic page cache?

davidhernandez’s picture

FileSize
100.62 KB
143.74 KB
183.95 KB
156.39 KB
135.59 KB

I checked the title in all the themes and the installer. This was all new installation. See attached files.

The only thing I found so far is that the contextual links to edit a node go a bit bonkers in Classy. I'm not sure why, since all the other themes are ok. Also, this only happens wit Basic page, NOT with Article. I wonder if it has to do with Basic having so few fields, and something coming out different in the markup. I didn't get a chance to look closely.

Here is a gif of what I saw: http://g.recordit.co/zGvJ7cg7G1.gif

I confirmed that the problem does not happen in head.

davidhernandez’s picture

To clarify, that flickering in the gif happens with the mouse curser still. I'm not moving it off the link and back on. The frame rate is rather slow in the gif.

davidhernandez’s picture

When I apply the patch on a current install this is what I see:

No title in Bartik.
Seven title is at the bottom of the page.
Classy has title.
Stark has title.

Run update.php
Maintenance page has title.

Seven has title in the correct location.
Bartik now has title and in correct location.

And I can see that all four themes have the title block set in the block configuration.

davidhernandez’s picture

Daniel was right to be cautious. We lose the title on the maintenance page. Is this because in maintenance mode blocks aren't active? Do we need to leave the variable printing there? Though that doesn't seem to be an issue with the installer.

davidhernandez’s picture

I'm revising part of #180. I do see the link problem with Articles, and in Stark. But as soon as I add a tag to an article node it goes away. I'm assuming there is something about having just the body field with content. Maybe the area is too short or something odd.

RainbowArray’s picture

I just looked at the history of #2005546: Use branding block in place of page template branding variables (site name, slogan, site logo). We are not using the branding block in the maintenance and install pages, because blocks are not available there. So... we'll have to figure out how to make this work without the page title block on those pages here too.

Page titles should be showing up without blocks module being enabled. I don't know why that's not happening on the maintenance page, but it is on the install page. That's weird.

RainbowArray’s picture

The page title is not displaying on the install page either. :/

RainbowArray’s picture

Issue summary: View changes
FileSize
65.66 KB
79.47 KB

Before

After

davidhernandez’s picture

I didn't think to look at the installed. I meant the maintenance page when running the updates. See here:

davidhernandez’s picture

Issue summary: View changes
FileSize
263.65 KB

In my screenshot and what I'm seeing is that the big "Drupal" is the page title. The "configure site" is an H1 but not the title? What is it?

davidhernandez’s picture

I see. I was looking at the wrong template. This is from Seven.

  <header role="banner">
    {% if site_name %}
      <h1 class="page-title">{{ site_name }}</h1>
    {% endif %}
  </header>

  {% if page.sidebar_first %}
    <aside class="layout-sidebar-first" role="complementary">
      {{ page.sidebar_first }}
    </aside>{# /.layout-sidebar-first #}
  {% endif %}

  <main role="main">
    {% if title %}
      <h1>{{ title }}</h1>
    {% endif %}
    {{ page.highlighted }}
    {{ page.content }}
  </main>

So the "Drupal" is just printing the site_name variable and the "Configure site" is title, but the page-title class is on the site name. Totally obvious. :/

Ok, so like maintenance mode the title isn't printing, because I think it can't make blocks at this stage.

I remember when we did the branding variables the variables had to be left so they could still work during the installation. I'm assuming this is the same case?

RainbowArray’s picture

Trying to address feedback.

#181: Seems like a good idea to check subject and body, but I'm not sure how to do that.
#182.2: Removed.
#182.3: We are no longer looking at #markup in the PageTitle render element, so I removed the #markup qualification from the comments here and in the RenderElement.#
#182.4: I don't know how to address that.
#182.5: Fixed.
#182.7: Added a test for the page title on a node page.
#182.8: Added space between breaks and new cases.
#182.9: I know Wim mentioned removing something with views_add_contextual_links, so maybe he can address that.
#182.10 & 11: Removed the @ingroup themable declarations from the Classy and Barik page-title templates.
#183.2: We used the same bit here in the branding block and local tasks and actions blocks updates. Not sure how to address these concerns.
#183.3: Updated. Again, that was used in the other block upgrade paths too.
#183.4: Hmm. I see that the space is gone.
#183.5: I'll address that in the next patch.

So areas where I could use help are:
- #181
- #182.4
- #182.9
- #183.2

RainbowArray’s picture

Status: Needs work » Needs review
FileSize
61.64 KB
2.86 KB

The maintenance and install pages ended up being a relatively painless fix. ['page']['#title'] has the correct title value in template_preprocess_maintenance page, so I assigned that to $variables['title'] and then we can still use it in just in the maintenance and install templates, which can't use blocks. Those are weird and frustrating templates, but whatever. Not a big deal. We did the same thing with branding block.

This also doesn't mean we need to lose sleep over the page title being in different places in relation to the main content and status messages for maintenance and install. Now all is back to how it used to be.

Status: Needs review » Needs work

The last submitted patch, 201: 2476947-201-convert-page-title-block.patch, failed testing.

The last submitted patch, 201: 2476947-201-convert-page-title-block.patch, failed testing.

RainbowArray’s picture

Reverting the change I made for 183.3 since it caused a test failure. Maybe there's something I'm not understanding, but this version appeared to work.

dawehner’s picture

We should better expand our test coverage to ensure that regression won't happen anymore in the future.

+++ b/core/modules/system/system.install
@@ -1706,9 +1706,9 @@ function system_update_8008() {
-        $name = 'block.block.{$theme_name}_page_title';
+        $name = sprintf('block.block.%s_page_title', $theme_name);
         $values = [
-          'id' => '{$theme_name}_page_title',
+          'id' => sprintf('%s_page_title', $theme_name),
           'region' => 'content',

Its just ugly. Maybe alternative just concat the strings normally

Wim Leers’s picture

  • #181: fixed.
  • #182.4 is definitely out of scope here.
  • #182.9: This is necessary because views_add_contextual_links() is neither injected nor namespaced; \Drupal\Tests\views\Unit\Plugin\Block\ViewsBlockTest() has the exact same work-around to let that unit test work.
  • #183.2: I don't understand what you are saying. We do the same in LocalActionsAndTasksConvertedIntoBlocksUpdateTest and SiteBrandingConvertedIntoBlockUpdateTest; so the same problem would need to be fixed there. Could we fix it in a separate issue, that happens in parallel to this one?
  • #183.3: fixed, by doing what #205 suggested.
dawehner’s picture

#183.2: I don't understand what you are saying. We do the same in LocalActionsAndTasksConvertedIntoBlocksUpdateTest and SiteBrandingConvertedIntoBlockUpdateTest; so the same problem would need to be fixed there. Could we fix it in a separate issue, that happens in parallel to this one?

Well, fact is that doing anything before runUpdates() is problematic. Do you mind filing a follow up?

We should better expand our test coverage to ensure that regression won't happen anymore in the future.

Do we also have a follow up for the title on the install/maintenance page, even well, I'd kinda argue that its in scope of this issue to ensure that the title always appears.

lauriii’s picture

Created follow-up for #182.4 and I think it can be worked later and be added in 8.1 etc. #2568035: Make it possible for blocks to opt-out from #lazy_builder callback

Status: Needs review » Needs work

The last submitted patch, 206: convert_page_title_to_block-2476947-206.patch, failed testing.

Wim Leers’s picture

Issue summary: View changes
Issue tags: -needs profiling
FileSize
10.92 KB
7.51 KB
227.52 KB

And now: profiling. Getting the flexibility of moving the title to any place by means of a block of course means some overhead, because in HEAD it is hardcoded. Moving from hardcoded to configurable always has a cost. So it's a matter of checking how big that cost is.

Profiling

This costs 1106 additional function calls, anon or auth, regardless of the page. Of course, only when Dynamic Page Cache is unable to cache a page.

Benchmarking

Setup
Fresh install, without (Dynamic) Page Cache: drush si; drush pmu -y page_cache; drush pmu -y dynamic_page_cache
Tested as anonymous user.
1000 sequential requests: ab -c 1 -n 1000 -g ab.tsv http://tres/contact/
Before
Requests per second:    12.97 [#/sec] (mean)
Time per request:       77.083 [ms] (mean)
Time per request:       77.083 [ms] (mean, across all concurrent requests)
Transfer rate:          276.22 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:    73   77   1.6     77      91
Waiting:       65   69   1.5     69      79
Total:         73   77   1.6     77      91

Percentage of the requests served within a certain time (ms)
  50%     77
  66%     77
  75%     78
  80%     78
  90%     79
  95%     80
  98%     81
  99%     82
 100%     91 (longest request)
After
Requests per second:    12.84 [#/sec] (mean)
Time per request:       77.893 [ms] (mean)
Time per request:       77.893 [ms] (mean, across all concurrent requests)
Transfer rate:          276.76 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:    73   78   1.7     78      85
Waiting:       65   70   1.7     70      77
Total:         73   78   1.7     78      85

Percentage of the requests served within a certain time (ms)
  50%     78
  66%     78
  75%     79
  80%     79
  90%     80
  95%     81
  98%     82
  99%     83
 100%     85 (longest request)
Histogram

Conclusion

A very small performance regression.

In my humble opinion, this is acceptable, since this is the very last block conversion, which will allow Drupal 8 to ship with a fully consistent page building model!

dawehner’s picture

Well, a 1% thing is something which sums up, just saying.

Wim Leers’s picture

DrupalCI came back green. PIFR did not. For the UMPTEENTH TIME: GET http://ec2-54-191-110-230.us-west-2.compute.amazonaws.com/checkout/user/login returned 0 (0 bytes). . ARRRGHHHH! Kill it with fire!

Re-testing…


#207:

Well, fact is that doing anything before runUpdates() is problematic. Do you mind filing a follow up?

Of course, done: #2568069: Block upgrade path tests refresh theme info before runUpdates().

Do we also have a follow up for the title on the install/maintenance page, even well, I'd kinda argue that its in scope of this issue to ensure that the title always appears.

Agreed. Done. This also revealed that the changes in #201 were in fact incorrect; the comment is correct, but the patch is not (and inconsistent). Fixed. The fail patch only contains the changes from the interdiff that do not affect Twig files.


#208: Thanks for filing that! Adding to related issues.

dawehner’s picture

Issue summary: View changes

The last submitted patch, 212: convert_page_title_to_block-2476947-211-FAIL.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 212: convert_page_title_to_block-2476947-211.patch, failed testing.

The last submitted patch, 212: convert_page_title_to_block-2476947-211-FAIL.patch, failed testing.

The last submitted patch, 212: convert_page_title_to_block-2476947-211.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
62.92 KB
1.07 KB

#2492839: Views replacement token bc layer allows for Twig template injection via arguments introduced that 1 fail here, probably because the changes in RolesRidArgument weren't fully kosher to begin with. That was the change I least understood, so it's great we no longer have to make any changes in that file :)

Rebased and fixed.

lauriii’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing

I've now reviewed the code few times and I don't see anything to complain anymore. I also think we can get over the 1% performance regression we have here.

Tested manually:
Title block appearing with upgrade path:

  • Bartik empty front page
  • Bartik node page
  • Seven
  • Classy node page

Title block appearing with clean install:

  • Bartik empty front page
  • Bartik node page
  • Seven
  • Classy node page

Also tried to remove title block from Bartik and title is still appearing on:

  • Bartik empty front page
  • Bartik node page
lauriii’s picture

Issue tags: +Needs change record

We could still use change record

Manuel Garcia’s picture

davidhernandez’s picture

Issue summary: View changes
Issue tags: -Needs change record

Added change record draft.

Anything in particular that needs updating in the issue summary? I deleted the todo. I didn't seem relevant any more and I deleting the line about adding a region.

davidhernandez’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs work
FileSize
9.21 KB

I used an existing install, checked all the normal things. Applied patch and ran update.php. I can confirm the page title is now fixed on the update page, which uses the maintenance page template.

Confirmed the title looks fine in Seven, Bartik, Classy, and Stark.

Confirmed the title appears in maintenance mode for Stark, Classy, and Seven.

The title does not show up for Bartik. It looks like the hard-coded printing of the variable was put back in the other templates, but not Bartik's.

Also, I still see the contextual link regression in Classy. To reproduce, apply patch, update or don't, and try to use the contextual link on a node. It appears to happen to any of the nodes I try. It isn't all the links, just the one that appears for the node's body. And on the node's page (node/1,) not the front page.

It looks like this. I get to links, but clicking either doesn't do anything.

Can anyone else verify?

davidhernandez’s picture

Issue summary: View changes
FileSize
130.17 KB

Also, I just noticed what Lauri said about disabling the page title block. The title does still appear on the page. It gets printed below the content. Why would we want this functionality? This seems bad. I can't make the title go away and it automatically appears in a bad spot. Why can I disable it, if it is being made some kind of requirement?

RainbowArray’s picture

I tested on simplytest.me with this patch. Created a node. On the node page, I see the two contextual links: the top is for the block, the bottom is for the node and allows for quick edit. Both worked fine for me. When I hovered over one of the two icons and clicked, I got the dropbutton and was able to click on any of the links in the dropbutton.

This is in the current version of Chrome on my Mac with Bartik.

In Classy, I'm seeing the behavior described in #224. Can hover over the contextual links on the body, but it's fiddly, and clicking on the contextual links does nothing.

Previously, there was one contextual link when hovering over the body with three options: quick edit, edit and delete.

Now there are three contextual links: one when hovering over the title, which only gives a configure block option; and two when hovering over the body. The first body contextual link has configure block, edit (which edits the node) and delete. The second has Quick Edit (which edits the title and body of the node), edit (which edits the node) and delete.

In an ideal world, it would be nice if the contextual link for the title had Configure Block, Quick Edit (for just the title or title + body), edit (for the node), and delete, and if hovering over the body provided one contextual link that did the same thing. That may well be out of scope for this issue, but wanted to note this, which seems like a potential UX improvement.

RainbowArray’s picture

Here's a screenshot from Dev Tools for Classy on a node that shows where the three contextual links are coming from.

The page title block has one div with data-contextual-id inside it.

The article with the main content has a div.contextual-region around it with a div.data-contextual-id, and the article also has a div.data-contextual-id inside it: both of those have the same value for the data-contextual-id. I have to think that duplication is likely causing at least part of the problem here.

By contrast here is the markup in Classy prior to this patch:

So this makes me think the problem is more to do with any changes to the main content area than to the page title block. Maybe the title_suffix that was added to the main content?

RainbowArray’s picture

As for the title appearing below the main content, that appears to happen in Bartik and Stark and Seven, but not Classy. In Classy the page title appears above the main content. I wonder what's different there?

We have a fallback in BlockPageVariant and SimplePageVariant to place the title in the content area if there is no title block placed anywhere. That's in part to help with tests where blocks cannot be enabled or to allow the title to appear if blocks aren't being used for whatever reason.

I would imagine something like Panels would have a different PageVariant where default title placement could be skipped if necessary? I'm not sure about that though.

RainbowArray’s picture

Status: Needs work » Needs review
FileSize
62.29 KB
640 bytes

This fixes the title on the maintenance page for Bartik.

I tried changing the page title weight on the render element placed if there's no block in BlockPageVariant and SimplePage Variant. Whether the weight is set at -900, -1000 or -1100, it still shows up below the main content, which seems really odd, especially because that does not happen in Classy. I don't see anything in the template or .theme for Classy that would make the page title work there but nowhere else.

I also tried removing the title_suffix from container--main-content. That did get rid of the double contextual link problem. If you place a main content block in Classy, there are still two context links in the body area, but they work.

But I thought there was a good reason for that title_suffix in container--main-content.

I'm not sure how to fix these issues, but maybe this gives Wim the info needed to come up with ideas for a solution.

dawehner’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests
+++ b/core/themes/bartik/templates/maintenance-page.html.twig
@@ -35,6 +35,9 @@
         <main id="content" class="column" role="main">
           <section class="section">
             <a id="main-content"></a>
+            {% if title %}
+              <h1 class="title" id="page-title">{{ title }}</h1>
+            {% endif %}
             {{ page.content }}

In other words we need tests for that as well

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
60.35 KB
8.42 KB

The root cause for #223/#226 is #174: the work-around I used to make a Page view's contextual links show up even when there is no main content block.

When there is a main content block, that automatically gets the contextual links from its contents. When there isn't, then in case of a Page view there is no template that prints {{ title_suffix }}. That's why I added container--main-content.html.twig and set that as the default theme wrapper for main content (at the cost of an extra wrapping <div>). But as a side effect of that, nodes (or any entity, really) gets their contextual links printed twice: once by the entity's template, and again by the container--main-content.html.twig theme wrapper.

Looking at this again, I found yet another possible work-around. Quoting #173:

[…] which included the hacks necessary for Drupal 7 (grep for view_array), and since then, the hacks have only grown more complex, mysterious and brittle (#1849822: Convert (HTML) view rendering to a render array, #2381277: Make Views use render caching and remove Views' own "output caching"). For example: we still have view_array, which itself was a hack around limitations in Drupal 7's theme system, added solely for the purpose of supporting contextual links, but then we don't ever populate it anymore in Drupal 8.

But the views-view.html.twig template does print a title suffix, it just relies on this view_array thing working correctly, so let's get that fixed instead. Then we can restore the old behavior for the main content block (no contextual links ever). I did keep the added test coverage that checks that the View has a contextual link both when the "main content" block is placed and when it is not. I merely adjusted the expectation (no "Configure block" link).

Did all that, and now it's working just like in HEAD.

RainbowArray’s picture

This adds a weight to the main content render array. The plus side is that if there are no blocks in the content region, the page title now appears above the main content. However, if there is a main content block but no page title block, the page title is still below.

Wim suggested turning off the #sorted flag on the $build for BlockPageVariant, which is a performance optimization. However, the sorting actually gets done in BlockRepositort->getVisibleBlocksPerRegion, with @uasort($assignment, 'Drupal\block\Entity\Block::sort'). So that isn't sorting any of the render arrays that might be placed in a region side by side with blocks. Frankly, I'm not sure how we go about doing that. But if we can, that will probably fix the order.

davidhernandez’s picture

Status: Needs review » Needs work

The last submitted patch, 231: 2476947-231-convert-page-title-block.patch, failed testing.

The last submitted patch, 231: 2476947-231-convert-page-title-block.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
61.61 KB
3.94 KB

I was able to figure out the root cause. I was right that it was the #sorted property in BlockPageVariant. @mdrummond just was setting it on the wrong level :) He was setting at the region level instead of the block level inside the region that contains all the fallback page. So close! :)

The reason that must be unset is because of:

      if (!empty($build[$region])) {
        // \Drupal\block\BlockRepositoryInterface::getVisibleBlocksPerRegion()
        // returns the blocks in sorted order.
        $build[$region]['#sorted'] = TRUE;
      }

… but when setting fallback main content/title/messages… that's no longer true! So when either of those are set, we must override that performance optimization again.

Furthermore, we must ensure #weight is actually set on the block render arrays so that we can sort them.

Those are the two only actual changes I made. I just fixed the unit tests to make the expectations match the changes in #231. And I made the changes in #231 slightly more elegant.


So, all that is now left, is #229.

RainbowArray’s picture

#236 adds a test for Bartik's maintenance page title, with a FAIL patch to demonstrate that the test works. At least hopefully it does.

In theory we could test any number of things with page titles across multiple themes, although I don't think that's something we typically do. That said, if adding this regression test helps, sure, no problem.

The last submitted patch, 236: 2476947-236-convert-page-title-block-FAIL.patch, failed testing.

The last submitted patch, 236: 2476947-236-convert-page-title-block-FAIL.patch, failed testing.

RainbowArray’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update, -Needs tests

We now have a green patch that addresses all feedback both from code reviews and manual testing. I've updated the issue summary. There's a change notice drafted. Let's get this in!

lauriii’s picture

Status: Needs review » Reviewed & tested by the community

Everything seems to be addressed since my latest RTBC. I tested this again manually and I can see that maintenance page has now the title and also the title is on top of the page after disabling the block.

davidhernandez’s picture

I'm still not sure I understand the title disabling thing? If you disable the block you still get a page title. Why can't disable the page title if I want to? This is a hard requirement? I'm thinking of things like not wanting the title on certain content types or the front page.

Wim Leers’s picture

I'm still not sure I understand the title disabling thing? If you disable the block you still get a page title. Why can't disable the page title if I want to? This is a hard requirement? I'm thinking of things like not wanting the title on certain content types or the front page.

Because we consider three things essential on a page:

  1. main content: without this, you literally would not be able to see anything that was supposed to show up at that URL
  2. title: without this, many pages become near meaningless, and a zillion tests would need to be updated to get the "title" block placed
  3. messages: without this, you wouldn't be able to see form validation errors etc.

Finally, both the main content and the title are things that are calculated for the current URL (route) you went to. They're both always calculated. Because they're both fundamental to the URL you're currently on.

Block module is core's layout system. If contrib adds additional ones, those can choose to not have fallback behavior, i.e. they can choose to NOT show the page title or the main content. But core needs to protect its users against shooting themselves in the foot. Without these 3 blocks, it's very easy to be either locked out (main content) or be completely lost how to make your site behave sanely again (title/messages).

davidhernandez’s picture

But that's not completely true. The node view doesn't have a title. You can create things without titles.

If someone wanted a page without a title, in the current system, there are straight forward ways to do that. If we are making the title a requirement of sorts, it is isn't consistant, and the UI is at least misleading. If you disable the block, it doesn't (to the unknowing eye) go away. If you alter the visibility settings on the block, they don't work. Limiting it to a specific content type, for example, controls the block visibility, but the default one then shows up at the top of content. To someone that doesn't understand the internals, this seems broken.

While all the normal settings are still there, the only thing you can actually do is move it around. That may actually be more limiting for a site builder than what we have now. At least someone could easily manipulate the title on the twig template, or even delete it. That isn't possible now. I think the most they could do is manipulate the contents of the page-title template, but that's hacky, and still leaves the block active.

Wim Leers’s picture

Alright. That's pretty convincing :)

I'm personally not at all opposed to that. Let's do that then. Reverting only the necessary parts of #170's interdiff to bring back the "place block" calls in the necessary places. And removing the fallback logic. This should not fail too badly, and hopefully will actually be green.

Status: Needs review » Needs work

The last submitted patch, 245: convert_page_title_to_block-2476947-245.patch, failed testing.

The last submitted patch, 245: convert_page_title_to_block-2476947-245.patch, failed testing.

RainbowArray’s picture

I went through the failing tests and placed the page title block where necessary.

Definitely agree people should have the ability to not have a page title with the UI. That does come up from time to time.

Wim Leers’s picture

Woohoo! Thanks Marc :)

+++ b/core/modules/aggregator/src/Tests/AggregatorRenderingTest.php
@@ -23,6 +23,12 @@ class AggregatorRenderingTest extends AggregatorTestBase {
+  protected function setUp() {

Nit: {@inheritdoc} on all these setUp() functions being added. We have plenty of those in core already though, so I think it's fine to not fix this unless we need another reroll.

RainbowArray’s picture

I wondered about that but saw a bunch of these without inheritdoc so left it off. Can certainly add that in if we need to.

Status: Needs review » Needs work

The last submitted patch, 248: 2476947-247-convert-page-title-block.patch, failed testing.

The last submitted patch, 248: 2476947-247-convert-page-title-block.patch, failed testing.

RainbowArray’s picture

Fixing remaining test failures.

RainbowArray’s picture

Status: Needs work » Needs review
Wim Leers’s picture

As far as I'm concerned, this is ready again. But I think it's up to lauriii or davidhernandez to re-RTBC :)

davidhernandez’s picture

Testing now:

Installed using head. Checked lots of pages with titles; nodes, views, front, etc. Made screenshots to check the markup after applying the patch.

Apply patch. Titles gone. Running update.php Update 8008 runs.

Title there for the update. Title block appears correctly in the block layout list.

Titles appear correctly in Seven on admin pages. Shortcut works correctly.

Bartik:
Node titles are good.
Quick edit on node page is good.
View page title is good.
View quick edit is good.
User page title is good.
Disabling title works correctly.
Limiting the title by content type works.
Moving the title to another region works.
Title appears on maintenance page.

Classy:
The page title appears at the bottom of the page. I assume this is because there is no "main page content" block set. The default order puts the title below. o we need to fix this? I corrected this by adding the main page content block and ordering the blocks.
after...
Node titles are good.
Quick edit on node page is good.
View page title is good.
View quick edit is good.
User page title is good.
Disabling title works correctly.
Limiting the title by content type works.
Moving the title to another region works.
Title appears on maintenance page.

Stark:
Stark has the same problem as Classy. By default the title appears at the bottom.

Something is wrong with the blocks. When I try to edit the page title block it thinks it's the branding block. This is the path it links to, /admin/structure/block/manage/stark_branding which results in page not found. I can't disable the block either. If I add another page title instance, I get admin/structure/block/manage/pagetitle_2 which is editable, but I can't do anything with the original.

Stopping here with this part of the testing. Stark seems unusable.

Doing a fresh install with the patch applied.

Oh wait, on a fresh install all the blocks are correctly there in Stark. Is the upgrade path ruining an existing site's block setup? I seem to be able to manipulate the blocks just fine now.

Trying an install and update again. In head Stark's block configuration looks fine. Running update...

So now Stark's block config looks ok. :/ The problem I described above with Classy is still the case. I'm not sure if there is some fragility with the update. ? Can someone else try, and with content. The site I was using was brand new, and I didn't use it for anything else, so I don't know where there was an issue with the blocks in Stark. The only difference is the first site had content before updating, the second did not. I'll try again with content.

Ok, the first thing I did was install, and then turn on Stark. The block config had all the right blocks, but then Seven started misbehaving and not showing tabs. I reinstalled, turned on Stark, and now no blocks appear for Stark. What is going on? Can someone else confirm this oddity? Is there something wrong in head?

RainbowArray’s picture

I found a couple bugs that might explain some of the oddities.

On BlockPageVariant we should remove the weight added to main content: that's not needed anymore.

There was also a bug with Seven's install config for the page title.

I checked all the others, and they looked okay. I checked the update hook, and it looked okay.

Might be worth another try now.

Status: Needs review » Needs work

The last submitted patch, 257: 2476947-257-convert-page-title-block.patch, failed testing.

The last submitted patch, 257: 2476947-257-convert-page-title-block.patch, failed testing.

RainbowArray’s picture

This should fix BlockPageVariantTest. Tests now pass locally.

RainbowArray’s picture

Manually tested on simplytest.me
- With Classy, title now appears above main content
- With Stark, title now appears above main content
- Editing blocks for Stark, this is the address I see for Stark's page title block: structure/block/manage/stark_page_title
- I am able to disable and enable the page title block in Stark
- I'm not seeing the oddity described with Seven and tabs

Since this is simplytest.me, I'm seeing a fresh install. I'll also try an upgrade locally.
- From 8.0.x HEAD, I did a fresh install.
- Then I switched to a branch with the latest patch applied.
- I cleared cache, then did an upgrade. I was advised of the page title upgrade.
- I installed Stark and set is as default.
- I checked all themes, and the page title block was correctly placed and seemed editable.

If you want to retest, David, maybe make sure to do a fresh install before you do the upgrade. Maybe you did that before, but that might help minimize any problems that could stem from testing other patches on your local. Maybe that wasn't what was going on, just a thought to eliminate variables.

mgifford’s picture

Issue summary: View changes
FileSize
9.33 KB

That's pretty neat. I really like how you can just push the title off into the sidebar like that with this patch.

What needs to be done to mark this RTBC?

I did notice this issue with Seven's titles if you try to access the block, well you can't:

This could be fixed in a follow-up issue though.

RainbowArray’s picture

I checked with LewisNyman, and in general there are some problems with contextual links in Seven. He agreed that for this it made sense to file a follow-up rather than trying to deal with that here, since larger issues are involved.

Wim Leers’s picture

Yep, #262 is a pre-existing problem.

davidhernandez’s picture

Status: Needs review » Needs work

Testing now. Starting with a fresh install using head. (todo: check minimal install)

Before
Checking default block configs for all four themes, saving screenshots for later comparison, noting existing styles and positions to check for visual regressions.

Bartik: Site branding, Main navigation, User account menu, Status messages, Breadcrumbs, Tabs, Help, Primary admin actions, Main page content, Search, Tools, User login, Footer menu, Powered by Drupal
Classy: none.
Seven: Primary tabs, Secondary tabs, Breadcrumbs, Status messages, Help, Primary admin actions, Main page content, User login
Stark: Secondary tabs, Primary admin actions, Main page content, User login, Primary tabs, Status messages, Help, Breadcrumbs

Checking all themes in maintenance mode, making screenshots.

Patch applied
Titles gone (as expected, pre update script). Checking blocks. Bartik same, Classy same, Stark same.

...running update.php. Title exists in the same place, styles are the same. 8008 runs cleanly.
Checking post update block configs. Bartik same with page title at the top of content, Classy same with page title the only block in content, Seven same with page title at top of header, Stark same with page title at the top of content.

Quickedit
Bartik: Quick edit links work for the front page view and each node on the front page. Works on the body of each node's page.
Classy: All work the same.
Seven: All work the same with Seven as the frontend theme.
Stark: In Stark the page title quickedit is not accessible. The button is available and clicking it creates the little drop down, but it drops below the available area. This might be a Stark styling thing, but the other small blocks with contextual links, like tabs, work fine. I'll come back to this on a fresh install of head to see if it works the same.

Title styling
Bartik is the same, Classy is the same, Seven is the same, Stark looks same.

Maintenance mode
Bartik is same, Classy is same, Seven is same, Stark is same.

Block visibility
Bartik: Removing from front only works, limiting to Article only works, moving to another region works, removing completely works, putting it back works.
Classy: Works.
Seven: Works.
Stark: Page title block config does not work. This is the same problem I saw from before. It is overriding the branding block.

+++ b/core/modules/system/system.install
@@ -1642,5 +1642,88 @@ function system_update_8007() {
+      case 'stark':
+        $name = 'block.block.stark_page_title';
+        $values = [
+          'id' => 'stark_branding',
+          'region' => 'content',
+        ] + $page_title_default_settings;
+        _system_update_create_block($name, $theme_name, $values);
+        break;

This is the problem.

edit: Those tags don't work, duh. The name and id are not the same.

davidhernandez’s picture

Ignore the 8007. That code is part of hte 8008 update, but the patch shows it because the function is added after.

davidhernandez’s picture

I'm checking install using head. The system branding block is not set in Stark on install, but it does appear in the update script. I'm assuming this is from the update when we converted the branding blocks. Is branding suppose to be part of Stark's default?

There is also something strange still going in head that I noticed the first time I tested this. On fresh installs using head when I enable Stark and set it as the default theme, the block layout can come out completely messed. Search, Footer menu, Tools, User login, Powered by Drupal, all end up in Left Sidebar. I haven't figure out what causes it. Sometimes when I install it happens, sometimes it doesn't. This is all in head, I drop the database and empty the files directory every time.

That obviously doesn't have anything to do with this issue, but I'm noticing it because well how many people are constantly installing and reinstalling and checking block layouts for Stark?

davidhernandez’s picture

Going back to the contextual link part of #265, all the contextual links I see work fine in Stark in head.

davidhernandez’s picture

Re: contextual links problem in Stark, this works fine on a new install using the patch. I assume it is related to the problem of the upgrade confusing the title block with the branding block, so this problem may solve itself with that fix.

davidhernandez’s picture

From what I can tell minimal install looks fine.

RainbowArray’s picture

Fixed the Stark upgrade for page title. After doing this, the contextual link for Stark's page title appears to work. Quick Edit worked for me too with Stark page title.

RainbowArray’s picture

For what it's worth, what #262 pointed out, that contextual links on Seven's page title aren't getting cut off, is caused because .content-header has overflow:hidden set on it. Remove that, and it works fine. This was never an issue before because in theory that region didn't have a contextual link that could overflow the region. And if there are tabs in that region, then there is enough vertical space that the contextual links won't overflow.

We'd need LewisNyman's feedback on that, though, as I'm not sure why that overflow hidden line is there. There might be a good reason for it.

Status: Needs review » Needs work

The last submitted patch, 271: 2476947-268-convert-page-title-block.patch, failed testing.

LewisNyman’s picture

Overflow hidden is required for tabs styling. It hides a border on desktop:

This is what happens on mobile when you disable it on mobile:

Hope this helps, I could spend some time trying to rework this implementation not to require overflow hidden but it is tricky because the mobile/desktop behaviour is tabs is so different, regressions are common.

davidhernandez’s picture

The contextual links issue in Seven is out of scope. It isn't created by this so we don't have to solve it here. There is also an issue to remove them completely in Seven.

davidhernandez’s picture

The fix in #271 fixes the problem I found with the Stark page title block and the Stark contextual link problem.

The Stark default block layout setup problem is completely bananas, but I'm having the problem with head, not this patch. So if we have a green patch I think it's finally good to go.

The last submitted patch, 271: 2476947-268-convert-page-title-block.patch, failed testing.

davidhernandez’s picture

That fails for me locally, so that's legit. Should the page_title_block placement be moved to the AggregatorTestBase setup()?

RainbowArray’s picture

Tested this fix locally, and it appears to fix the test. Looks like a new test was added that checked the title, so we just needed to place the title block for the test. No worries.

If this passes, maybe back to RTBC?

RainbowArray’s picture

Status: Needs work » Needs review
davidhernandez’s picture

Status: Needs review » Reviewed & tested by the community

I tested the change in #271 previously, and it was good to me. #279 fixes the test.

star-szr’s picture

I'm partway through reviewing this, I may post a nitfixes only patch because that's all I'm finding so far.

One thing that may be disputable, so posting now:

+++ b/core/lib/Drupal/Core/Display/PageVariantInterface.php
@@ -36,4 +36,15 @@
+   *   The page title: either a string for plain titles or a #markup render
+   *   array for formatted titles.

I'm curious why this says "a #markup render array". Can't it just be any kind of render array?

In other words: Can we just copy and paste the comment from the implementation?

star-szr’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
80.81 KB
3.59 KB
+++ b/core/modules/views/tests/src/Unit/Routing/ViewPageControllerTest.php
@@ -181,3 +181,13 @@ public function testHandleWithArgumentsOnOverriddenRouteWithUpcasting() {
+
+}
+
+namespace {
+  // @todo replace views_add_contextual_links()
+  if (!function_exists('views_add_contextual_links')) {
+    function views_add_contextual_links() {
+    }
+  }
+}

Does this need an issue created and linked for the todo? Or does this need to be handled here?

The rest I found is all minor/nitpicks, see below and/or attached interdiff.


  1. +++ b/core/lib/Drupal/Core/Block/Plugin/Block/PageTitleBlock.php
    @@ -0,0 +1,55 @@
    + * @Block(
    + *   id = "page_title_block",
    + *   admin_label = @Translation("Page title")
    + * )
    

    Added a trailing comma here.

  2. +++ b/core/modules/aggregator/src/Tests/AddFeedTest.php
    @@ -14,6 +14,12 @@
    +
    +  protected function setUp() {
    +    parent::setUp();
    +    $this->drupalPlaceBlock('page_title_block');
    +  }
    +
    
    +++ b/core/modules/aggregator/src/Tests/AggregatorRenderingTest.php
    @@ -23,6 +23,12 @@ class AggregatorRenderingTest extends AggregatorTestBase {
    +  protected function setUp() {
    +    parent::setUp();
    +
    +    $this->drupalPlaceBlock('page_title_block');
    +  }
    +
    

    Made these consistent (added blank line to AddFeedTest::setUp(), this patch adds more ones like the second one and core in general seems to like adding a blank line after the parent method call).

  3. +++ b/core/modules/block/src/BlockViewBuilder.php
    @@ -99,12 +100,13 @@ public function viewMultiple(array $entities = array(), $view_mode = 'full', $la
             ],
    +        '#weight' => $entity->getWeight()
           );
    

    Added trailing comma here.

  4. +++ b/core/modules/block/tests/src/Unit/Plugin/DisplayVariant/BlockPageVariantTest.php
    @@ -74,28 +74,32 @@ public function setUpDisplayVariant($configuration = array(), $definition = arra
           'block5' => array(
    -        'center', FALSE, TRUE,
    +        'center', FALSE, TRUE, FALSE,
    +      ),
    +      // Test a block implementing TitleBlockPluginInterface.
    +      'block7' => array(
    +        'center', FALSE, FALSE, TRUE,
           ),
    

    Changed all instances of block7 in this file to block6, this seems to go back to #64 but I don't see anything that means we need this gap in the sequence :)

davidhernandez’s picture

The todo was copied with the code from the ViewsBlockTest. So it doesn't need to be handled here.

RainbowArray’s picture

Patch no longer applied. Here's a quick reroll. Thanks for the review and fixes, Cottser!

RainbowArray’s picture

Also, thanks for making the fix for #282. Agree with the change: does not need to be a #markup render array.

davidhernandez’s picture

Added follow up for the todo. #2571679: Replace views_add_contextual_links() in views.module with service

That link needs to be added to the comments in ViewPageControllerTest and ViewBlockTest.

davidhernandez’s picture

Adding links for the todos.

The last submitted patch, 283: convert_title_page-2476947-283.patch, failed testing.

davidhernandez’s picture

Status: Needs review » Reviewed & tested by the community

I think we are back to ready.

catch’s picture

Issue tags: +rc deadline

Adding RC deadline tag, at least for now that seems like the right cut-off point, although also, last one! Not able to do a full review atm.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 288: convert_title_page-2476947-288.patch, failed testing.

RainbowArray’s picture

#2458763: Remove the ability to configure a block's cache max-age went in which removed the necessity and ability to set max age cache settings on blocks, which is likely what caused all the errors.

This fixes that.

There are still max age settings on the local action and tasks block upgrade paths. That should maybe be changed in a follow-up?

davidhernandez’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 293: 2476947-293-convert-page-title-block.patch, failed testing.

The last submitted patch, 293: 2476947-293-convert-page-title-block.patch, failed testing.

Manuel Garcia’s picture

Curse the Gods! >:-(

The last submitted patch, 293: 2476947-293-convert-page-title-block.patch, failed testing.

RainbowArray’s picture

Fails are almost assuredly due to #2567183: Re-export all built-in configuration in core going in. Probably not too bad a fix. Need to redo config files.

RainbowArray’s picture

Manuel Garcia’s picture

Thanks @mdrummond for getting this back to green!!

+++ b/core/profiles/minimal/config/install/block.block.stark_page_title.yml
@@ -1,16 +1,17 @@
+provider: null
...
   provider: core

+++ b/core/profiles/standard/config/install/block.block.bartik_page_title.yml
@@ -1,16 +1,17 @@
+provider: null
...
   provider: core

+++ b/core/profiles/standard/config/install/block.block.classy_page_title.yml
@@ -1,16 +1,17 @@
+provider: null
...
   provider: core

+++ b/core/profiles/standard/config/install/block.block.seven_page_title.yml
@@ -1,16 +1,17 @@
+provider: null
...
   provider: core

Not very clear as to what changed here, but shouldn't the provider be core?

RainbowArray’s picture

You would think, but if you do an actual config export that is what you get. Same is true for other similar blocks.

Manuel Garcia’s picture

Status: Needs review » Reviewed & tested by the community

OK then, back to RTBC

Wim Leers’s picture

And that is guaranteed to be correct thanks to #2567183: Re-export all built-in configuration in core having introduced test coverage that verifies all config is accurate.

Fingers crossed this will land soon :)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 301: 2476947-301-convert-page-title-block.patch, failed testing.

mgifford’s picture

Issue tags: +Needs reroll
RainbowArray’s picture

RainbowArray’s picture

RainbowArray’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
81.42 KB

Quick little reroll. Conflicts in HtmlRenderer and TestDisplayVariant due to the issues mentioned above. Nothing that took much sorting out. Move along, move along.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC.

Quotspection...

Fingers crossed this will land soon :)

Fingers crossed this will land soon :)

Fabianx’s picture

RTBC + 1

dsnopek’s picture

RTBC +1

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 310: 2476947-310-convert-page-title-block.patch, failed testing.

RainbowArray’s picture

Status: Needs work » Needs review
FileSize
81.42 KB

Straightforward reroll. Didn't even need to resolve conflicts. So this should be insta-RTBC-able as soon as tests are green.

Status: Needs review » Needs work

The last submitted patch, 315: 2476947-315-convert-page-title-block.patch, failed testing.

The last submitted patch, 315: 2476947-315-convert-page-title-block.patch, failed testing.

RainbowArray’s picture

Status: Needs work » Needs review
FileSize
81.42 KB
540 bytes

Duplicate system update number. Surprised that didn't show up as a conflict during the reroll.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC.

Quotspection...

Fingers crossed this will land soon :)

Fingers crossed this will land soon :)

Fingers crossed this will land soon :)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 318: 2476947-318-convert-page-title-block.patch, failed testing.

The last submitted patch, 318: 2476947-318-convert-page-title-block.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
82.2 KB
886 bytes

#2575605: Field delete form does not display configuration dependencies that will be updated/deleted added FieldUIDeleteTest::fieldUIDeleteField(), which is failing. This indicates we must also place the title block in that new test.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 322: 2476947-322-convert-page-title-block.patch, failed testing.

dsnopek’s picture

Status: Needs work » Reviewed & tested by the community

Passing now...

catch’s picture

Issue tags: +beta target

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 322: 2476947-322-convert-page-title-block.patch, failed testing.

Fabianx’s picture

Status: Needs work » Reviewed & tested by the community

Back to RTBC

  • catch committed bef7274 on 8.0.x
    Issue #2476947 by mdrummond, Wim Leers, davidhernandez, lauriii,...
catch’s picture

Status: Reviewed & tested by the community » Fixed

I just reviewed this, and nothing stuck out. Upgrade path I spent a bit more time reviewing and that looks great.

+++ b/core/modules/views/views.theme.inc
@@ -38,6 +38,18 @@ function template_preprocess_views_view(&$variables) {
+  // contextual_preproces() only works on render elements, and since this theme

contextual_preprocess() - can be fixed on commit. Did that.

This was on webchick's list for before the beta, but I don't see comments on this issue. I was going to leave it open for a few more hours to give her time to look if she wanted to review first, but then I realised it's Wednesday and ideally we'd release the beta today or tomorrow.

So I think it makes sense to get this in sooner than later - any fallout we have a bit of time to deal with, and this is the very last non-block page variable.

Committed/pushed to 8.0.x, thanks!

davidhernandez’s picture

*throws confetti*

dsnopek’s picture

Huzzah! Thanks, everyone! :-)

webchick’s picture

Totally fine not to hold it up on me. :) I was just in contact with the themers and wanted to raise this as a concern.

Jeff Burnz’s picture

I'm so happy I'm crying tears of joy to see this in, just amazing and wonderful, so happy :)

Anonymous’s picture

Where does the wrapping div with "field field--name-name field--type-string field--label-hidden field__item" classes coming from? I just cannot find the source of this damn thing.

---

The <div class="field field--name-name field--type-string field--label-hidden field__item">Some title</div> string is already present in the preprocess hook!!

dawehner’s picture

Where does the wrapping div with "field field--name-name field--type-string field--label-hidden field__item" classes coming from? I just cannot find the source of this damn thing.

Install beta 16, it was there always. See \Drupal\Core\Entity\Controller\EntityViewController::buildTitle

Anonymous’s picture

I jumped from beta 15 to RC1 so this is new to me. Anyway, how do I get rid of it? As I have mentioned in the preprocess hooks it is already provided as string so the only way is to use regex which is stupid.

---

Behavior of this thing is very weird and inconsistent. On taxonomy term page I am getting the aforementioned div, on my custom entity page I am getting nothing(ie proper
label

), on node page I am getting span(
label

).

This does not make any sense from developer nor themer point of view. How can I standardize what is being outputted??

Wim Leers’s picture

#338: this issue introduced #type => 'page_title' and page-title.html.twig. You can implement hook_preprocess_page_title().

I think that's what you're getting at.

davidhernandez’s picture

This should all be coming from the template. It sounds like you are using Classy, and that wrapper and class names come from the field--node--title template. That template wasn't introduced here, so you should have been seeing that wrapper on a node title for some time.

rootwork’s picture

I think Wim has it right -- it's the new page-title.html.twig template. Took me a minute to figure that out too -- though having Twig debugging on helped, as it showed me right where it was!

I've been loving this new implementation of the page title -- haven't run into the issues in #338 but maybe it all stems from that new template.

Anonymous’s picture

No, the template nor preprocess function has no say in this. The title is already pre-rendered html string since it is rendered in \Drupal\Core\Entity\Controller\EntityViewController::buildTitle

davidhernandez’s picture

You asked where does the wrapping span tag and field level CSS classes come from. That is in the field template. The generic H1 on pages where the page title is not a field comes from the page-title template. buildTitle calls the renderer but it does not control the markup or classes.

Status: Fixed » Closed (fixed)

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

cilefen’s picture