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.

Also the current solution makes tabs uncacheable by render cache unless whole page could be cached. Tabs are also being generated even though they wouldn't be printed.

Proposed solution

  • Introduce them as specific blocks that can be moved around.
  • Remove special case items from themes.
  • Introduce regions as needed to place these elements.

Performance gain (see #264):

(block management page which has action links, primary tabs and secondary tabs. Tested as anonymous user. Warm caches but internal page caching turned off.)

=== 8.0.x..cached-tabs compared (55919cdb0fed9..55919d1190958):

ct  : 92,936|88,242|-4,694|-5.1%
wt  : 159,482|151,346|-8,136|-5.1%
mu  : 18,570,072|17,711,328|-858,744|-4.6%
pmu : 19,736,144|18,835,248|-900,896|-4.6%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=55919cdb0fed9&...

Tasks remaining

-Review it!
-Commit!

Related issues

The code should depend on blocks as plugins (#1535868: Convert all blocks into plugins), however, we should not mark this postponed so we can do work on this in parallel and be ready for commits once that lands.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task
Issue priority Major because it significantly improves the ability for site builders to manage page elements from the UI. Not critical because nothing is broken.
Unfrozen changes Unfrozen because it mostly affects the page template, and templates are not frozen.
Prioritized changes The main goal of this issue is to improve site building and theming experience. It is also a continuation of work that was done pre-beta to convert all page template variables to blocks. This issue is also performance improvement which is prioritized change.
Disruption There will be possible disruption to sites with customized versions of the templates affected, or depending on the variables that were removed/moved.
Files: 
CommentFileSizeAuthor
#282 convert_page_elements-507488-282.patch74.34 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,259 pass(es).
[ View ]
#276 interdiff.txt3.22 KBlauriii
#276 convert_page_elements-507488-276.patch74.58 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,265 pass(es).
[ View ]
#274 interdiff.txt1.33 KBlauriii
#274 convert_page_elements-507488-274.patch75.92 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,373 pass(es).
[ View ]
#272 interdiff.txt22.91 KBlauriii
#272 convert_page_elements-507488-272.patch75.92 KBlauriii
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,359 pass(es), 14 fail(s), and 0 exception(s).
[ View ]
#263 interdiff.txt5.31 KBlauriii
#263 convert_page_elements-507488-263.patch76.61 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,717 pass(es).
[ View ]
#252 localactionsblock.png18.54 KBdavidhernandez
#250 interdiff.txt2.98 KBlauriii
#250 convert_page_elements-507488-250.patch76.31 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,276 pass(es).
[ View ]
#247 interdiff.txt2.33 KBlauriii
#247 convert_page_elements-507488-247.patch78.01 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,368 pass(es).
[ View ]
#243 interdiff.txt3.02 KBlauriii
#243 convert_page_elements-507488-243.patch77.99 KBlauriii
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,381 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#240 interdiff.txt2.41 KBlauriii
#240 convert_page_elements-507488-239.patch76.35 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,368 pass(es).
[ View ]
#237 interdiff.txt6.37 KBlauriii
#237 convert_page_elements-507488-237.patch76.37 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,371 pass(es).
[ View ]
#235 interdiff.txt3.23 KBlauriii
#235 convert_page_elements-507488-235.patch76.1 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,363 pass(es).
[ View ]
#233 bartik_after.png2.82 KBbill richardson
#233 bartik_before.png7.08 KBbill richardson
#232 interdiff.txt650 byteslauriii
#232 convert_page_elements-507488-232.patch77.52 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,326 pass(es).
[ View ]
#230 interdiff.txt9.53 KBlauriii
#230 convert_page_elements-507488-230.patch78.03 KBlauriii
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,328 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#223 interdiff.txt3.14 KBlauriii
#223 convert_page_elements-507488-223.patch78.2 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,339 pass(es).
[ View ]
#219 interdiff.txt7.72 KBlauriii
#219 convert_page_elements-507488-219.patch78.38 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,320 pass(es).
[ View ]
#218 interdiff.txt1015 byteslauriii
#218 convert_page_elements-507488-218.patch75.6 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,306 pass(es).
[ View ]
#216 interdiff.txt406 byteslauriii
#216 convert_page_elements-507488-216.patch74.47 KBlauriii
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,280 pass(es), 5 fail(s), and 0 exception(s).
[ View ]
#214 interdiff.txt6.23 KBlauriii
#214 convert_page_elements-507488-214.patch75.02 KBlauriii
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/tests/Drupal/Tests/Core/Menu/LocalActionDefaultTest.php.
[ View ]
#211 convert_page_elements-507488-211.patch70.22 KBlauriii
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#209 interdiff.txt49.51 KBlauriii
#209 convert_page_elements-507488-209.patch70.31 KBlauriii
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch convert_page_elements-507488-209.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#207 interdiff.txt468 byteslauriii
#207 convert_page_elements-507488-207.patch63.26 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,254 pass(es).
[ View ]
#204 interdiff.txt3.92 KBlauriii
#204 convert_page_elements-507488-203.patch63.26 KBlauriii
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,249 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#201 interdiff.txt1.08 KBlauriii
#201 convert_page_elements-507488-201.patch60.42 KBlauriii
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,115 pass(es), 24 fail(s), and 0 exception(s).
[ View ]
#199 interdiff.txt6.85 KBlauriii
#199 convert_page_elements-507488-199.patch60.41 KBlauriii
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,098 pass(es), 26 fail(s), and 16 exception(s).
[ View ]
#197 interdiff.txt17.74 KBtuutti
#197 convert_page_elements-507488-195.patch57.96 KBtuutti
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,038 pass(es), 31 fail(s), and 21 exception(s).
[ View ]
#192 interdiff.txt613 byteslauriii
#192 convert_page_elements-507488-192.patch40.68 KBlauriii
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 93,871 pass(es), 194 fail(s), and 28 exception(s).
[ View ]
#189 interdiff.txt11.15 KBlauriii
#189 convert_page_elements-507488-187.patch39.97 KBlauriii
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,506 pass(es), 198 fail(s), and 37 exception(s).
[ View ]
#182 interdiff.txt9.88 KBlauriii
#182 convert_page_elements-507488-182.patch39.5 KBlauriii
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,901 pass(es), 192 fail(s), and 7 exception(s).
[ View ]
#180 interdiff.txt1.02 KBlauriii
#180 convert_page_elements-507488-180.patch28.62 KBlauriii
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,821 pass(es), 279 fail(s), and 73 exception(s).
[ View ]
#177 interdiff.txt1.78 KBlauriii
#177 convert_page_elements-507488-177.patch29.03 KBlauriii
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#175 interdiff.txt851 byteslauriii
#175 convert_page_elements-507488-175.patch26.65 KBlauriii
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#173 interdiff.txt4.13 KBlauriii
#173 convert_page_elements-507488-171.patch26.82 KBlauriii
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#165 convert_page_elements-507488-165.patch26.83 KBborisson_
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#159 interdiff.txt1.5 KBManuel Garcia
#159 convert_page_elements-507488-159.patch26.83 KBManuel Garcia
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch convert_page_elements-507488-159.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#156 interdiff.txt11.52 KBManuel Garcia
#156 507488-156-action-tabs-into-blocks.patch27.02 KBManuel Garcia
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#152 interdiff.txt1.61 KBManuel Garcia
#152 507488-152-action-tabs-into-blocks.patch24.48 KBManuel Garcia
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]
#147 507488-147-action-tabs-into-blocks.patch24.48 KBManuel Garcia
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/modules/system/src/Plugin/Block/SystemPageActionsBlock.php.
[ View ]
#137 convert-page-elements-blocks-507488-137.patch43.23 KBWim Leers
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,809 pass(es), 517 fail(s), and 328 exception(s).
[ View ]
#133 interdiff.txt3.49 KBManuel Garcia
#133 convert-page-elements-blocks-507488-132.patch35.85 KBManuel Garcia
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,752 pass(es), 563 fail(s), and 330 exception(s).
[ View ]
#131 interdiff.txt5.75 KBManuel Garcia
#131 convert-page-elements-blocks-507488-131.patch33.47 KBManuel Garcia
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,701 pass(es), 542 fail(s), and 494 exception(s).
[ View ]
#126 interdiff.txt11.33 KBManuel Garcia
#126 convert-page-elements-blocks-507488-126.patch29.5 KBManuel Garcia
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,938 pass(es), 516 fail(s), and 491 exception(s).
[ View ]
#125 interdiff.txt2.36 KBManuel Garcia
#125 convert-page-elements-blocks-507488-125.patch18.17 KBManuel Garcia
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,794 pass(es).
[ View ]
#123 convert-page-elements-blocks-507488-123.patch18.28 KBManuel Garcia
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,297 pass(es).
[ View ]
#120 convert-page-elements-blocks-507488-120.patch18.27 KBManuel Garcia
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/modules/system/src/Block/SystemPageActionsBlock.php.
[ View ]
#118 convert-page-elements-blocks-507488-118.patch18.27 KBManuel Garcia
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/modules/system/src/Block/SystemPageActionsBlock.php.
[ View ]
#94 507488-94-convert-page-elements-blocks.patch18.27 KBeiriksm
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 507488-94-convert-page-elements-blocks.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#90 507488-90-convert-page-elements-blocks.patch18.62 KBWim Leers
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,168 pass(es).
[ View ]
#11 page-elements-as-blocks-11.patch2.68 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 56,817 pass(es).
[ View ]
#1 new-blocks.patch7.23 KBmaximiliam
Failed: 11147 passes, 428 fails, 2 exceptions
[ View ]

Comments

maximiliam’s picture

StatusFileSize
new7.23 KB
Failed: 11147 passes, 428 fails, 2 exceptions
[ View ]

Here is an initial patch that implements this blocks. After applying the patch and clearing the cache, go to admin/build/block and put the blocks called "Page title", "Primary tabs", "Secondary tabs" and/or "Messages" in the block region called "Title" or in any region you prefer.

When the "Page title" block is enabled and the "Primary tabs" block is disabled, the primary tabs will be shown on the same line as the page title.

maximiliam’s picture

Status:Active» Needs review

Status:Needs review» Needs work

The last submitted patch failed testing.

maximiliam’s picture

Status:Needs work» Active
sun.core’s picture

Version:7.x-dev» 8.x-dev
stevector’s picture

Status:Active» Closed (duplicate)

Major changes along these lines are in progress at http://drupal.org/sandbox/eclipsegc/1441840

David_Rothstein’s picture

Status:Closed (duplicate)» Needs work

This is a relatively small, focused issue, so it should remain open.

But see also #1053648: Convert site elements (site name, slogan, site logo) into blocks which is closely related. Although they focus on "blockify-ing" different things, it may make sense to merge them.

That issue also points out the existence of the Blockify module, which is relevant here.

David_Rothstein’s picture

Title:Title and tabs as blocks» Convert the page title, tabs, and messages into blocks
sun’s picture

Component:base system» theme system
Issue tags:+Blocks-Layouts
Gábor Hojtsy’s picture

Issue tags:+B&L

Tagging it up.

Gábor Hojtsy’s picture

Title:Convert the page title, tabs, and messages into blocks» Convert page elements (title, tabs, actions, breadcrumb, messages) into blocks
StatusFileSize
new2.68 KB
PASSED: [[SimpleTest]]: [MySQL] 56,817 pass(es).
[ View ]

All right, so given #1535868: Convert all blocks into plugins, it probably does not make much sense at this point to convert these to blocks as blocks exist pre-plugins. However, @EclipseGC actually did lots of the conversion work that is involved with these blocks in interim patches at #1787846: Themes should declare their layouts. It was not actually related to introducing layouts as a concept, and I want to make sure we have the patch around, so here it is. All the code included is from @EclipseGC.

Blocks included:
- local actions
- breadcrumb
- messages

Page title and tabs as in the issue title are not yet included. Following the same pattern, we should be able to easily deduct the rest of the blocks though.

Technically this issue does not make much sense until #1535868: Convert all blocks into plugins however, I think it would make sense to parallelise work.

Gábor Hojtsy’s picture

Status:Needs work» Needs review
sun’s picture

Issue tags:-B&L
+++ b/core/modules/layout/lib/Drupal/layout/Plugin/block/block/PageBreadcrumb.php
@@ -0,0 +1,30 @@
+      '#children' => theme('breadcrumb', array('breadcrumb' => drupal_get_breadcrumb())),

+++ b/core/modules/layout/lib/Drupal/layout/Plugin/block/block/PageMessages.php
@@ -0,0 +1,30 @@
+      '#children' => theme('status_messages'),

I think we should use #theme here, no?

The other problem is that theme_status_messages() needs to be invoked as late as possible in the rendering process, which is currently achieved and guaranteed by invoking it from template_process_page(); i.e., after the entire page has been composed.

The same also applies to the breadcrumb... if it's rendered too early, then modules do not have a chance to override it for particular pages.

I wonder whether the blocks/layout system has a concept for these late-rendering cases?

[Also removing duplicate tag.]

EclipseGc’s picture

So, I generally agree, we should move these to #theme for the time being. I generally trying to stay as true to whatever the code was already doing when I do these conversion. I don't always do that depending on the circumstances and I honestly can't remember what was happening here, but in any event, moving to #theme should leave these as render arrays for as long as possible and defer sun's questions until we move to blocks returning strings.

To address your question sun, yes in panels today we have a render order of sorts in place. Basically (if I recall) it renders in order, except for a list that may be tagged for "render last" in which case they render after everything else has rendered. This is useful in situations where you have something like solr facets that won't return data until the corresponding query to the solr engine has been run (which the facet itself wouldn't do by say a pager generated from that query would). I don't really like that particular behavior as I'd really like there to be a phase that happens before rendering that executes any sort of stuff that the blocks will need for rendering. It's at this level that I would hope module could interact with blocks. That level has not been specifically built in the blocks patch yet because it has more to do with layouts than it does blocks.

TL:DR; You bring up a good point, I think we can defer to by switching to #theme for the time being, and worrying about it once blocks and layouts/panelsy controller are all in core together.

Eclipse

andypost’s picture

ctools has "render last" for that purpose

+1 to have this special setting for blocks, contrib has a lot of breadcrumb overrides so this really needs to render after other blocks

Suppose "old-page-level" blocks should have "render last" priority by default most of them are config, but content-related blocks should be rendered first

EclipseGc’s picture

No, that's not what I'm suggesting. I think there should be an explicit phase where modules can affect this stuff before blocks are rendered. I'm not sure I see the purpose in delaying the rendering when there's an explicit phase to support these sorts of alterations. Likewise I would prefer individual blocks to never need to worry about their rendering order. If there's something a block depends on that another block generates or whatever, then that should actually all take place in the phase I'm discussing. Again I think solr facets are a good example of this.

Also, as an aside, the hook_block_alter that the blocks as plugins patch introduces gives module developers the ability to fully replace the class for any block, so if a module really needed complete control over a breadcrumb, they could take it.

Eclipse

Gábor Hojtsy’s picture

I propose we group at #1053648: Convert site elements (site name, slogan, site logo) into blocks first which is much simpler, but still needs to set up the pattern of introducing the blocks, new regions as needed, removal of existing theme variables, blocks added in install profiles, etc. That pattern can be replicated here and in #1869476: Convert global menus (primary links, secondary links) into blocks but both this one and the menu one need more discussion as to what we do with associated specific problems, while the logo/slogan and site name could work flawlessly.

xjm’s picture

Category:feature» task
larowlan’s picture

sdboyer’s picture

just noting that this is a blocker for SCOTCH, and the work i'm currently doing in our princess branch.

it may be that this is easier to finish up in princess directly because we don't have to deal with some of the architectural awkwardness (like special regions). if that's the case, i may just pull the base work in there and go forward with it.

webchick’s picture

This is another one of those issues that it'd be great to have as a patch separate from the princess branch, because it has value outside of SCOTCH.

xjm’s picture

Priority:Normal» Major
benjifisher’s picture

#11: page-elements-as-blocks-11.patch queued for re-testing.

Gábor Hojtsy’s picture

Title:Convert page elements (title, tabs, actions, breadcrumb, messages) into blocks» Convert page elements (title, tabs, actions, messages) into blocks

Breadcrumbs were already converted in http://drupalcode.org/project/drupal.git/commitdiff/4d08d57418c663b59953... The rest needs to be converted.

andypost’s picture

Is this issue still valid for d8?

Gábor Hojtsy’s picture

I think this would be an API change for themers, so it is dependent on maintainers agreeing it should be done. I think it would be amazing to still do this.

jibran’s picture

Status:Needs review» Needs work

NW as per #24.

Jeff Burnz’s picture

Yes this is still relevant (I don't see this has having been done anywhere else as yet) and yes we should defintily do this.

Will these now move into system module?

EclipseGc’s picture

That sounds like the right place for them.

Eclipse

EclipseGc’s picture

Issue summary:View changes

Update summary.

klonos’s picture

joelpittet’s picture

I made an interim solution a while ago to solve the early rendering issue. RenderWrapper class just takes the function and let's the __toString() call the function with it's arguments as late as possible. You can see that in core (sorry I hate the name I came up with but nothing came to me and still hasn't). Change Notice here https://drupal.org/node/2052023

Maybe this can be re-rolled with that on there.

theme() is being deprecated. #2006152: [meta] Don't call theme() directly anywhere outside drupal_render()

mdrummond’s picture

Made some progress on #1053648: Convert site elements (site name, slogan, site logo) into blocks. Hopefully that pattern can be useful to taking care of this issue too.

mdrummond’s picture

Status:Needs work» Needs review
StatusFileSize
new5.89 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,481 pass(es).
[ View ]

Trying to get this issue restarted after a long hiatus.

This first patch just has the page title conversion.

The good thing is that you can now go to the block layout page and request a page title block. Yay!

The bad news is that it doesn't display anything. Boo.

I think I'm missing a step where the page variables are set for the templates in preprocess. Couldn't figure that out. But posting this as a progress report.

I'll probably get started on adding in the other page elements as block plugins later tonight.

If you spot what's wrong, feel free to let me know!

mdrummond’s picture

Thanks to some help from chx and larowlan on IRC, I got this working.

One thing I'm not certain about is if title_prefix and title_suffix will work with this. I'm not explicitly generating those and sending them into the template, so I'm guessing not? I couldn't find where those were generated for the page template. If anybody has an example I can look at, I'll try to work that in.

In the meantime I'll try to work in some of the other page elements.

mdrummond’s picture

StatusFileSize
new7.51 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,499 pass(es).
[ View ]
new3.94 KB

Helps if I post the patch.

mdrummond’s picture

StatusFileSize
new12.3 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,499 pass(es).
[ View ]
new18.37 KB

The page actions block is now working. I also retitled the title block from SystemTitleBlock to SystemPageTitleBlock, as I think that's a bit clearer.

joelpittet’s picture

@mdrummond thanks for picking this up again. Small nitpick, mind using 2 spaces for the indenting in the twig templates?

tim.plunkett’s picture

  1. +++ b/core/modules/system/src/Plugin/Block/SystemPageActionsBlock.php
    @@ -0,0 +1,106 @@
    +    $action_links = menu_get_local_actions();

    I think you could inline this method here, it'd mean injecting the relevant services.

  2. +++ b/core/modules/system/src/Plugin/Block/SystemPageActionsBlock.php
    @@ -0,0 +1,106 @@
    +    $form['cache']['#description'] = t('This block is never cacheable, it is not configurable.');

    Here and elsewhere, $this->t()

  3. +++ b/core/modules/system/src/Plugin/Block/SystemPageTitleBlock.php
    @@ -0,0 +1,134 @@
    +    if ($route = $request->attributes->get(\Symfony\Cmf\Component\Routing\RouteObjectInterface::ROUTE_OBJECT)) {

    Have a use statement above, please

  4. +++ b/core/modules/system/templates/block--system-page-actions-block.html.twig
    @@ -0,0 +1,17 @@
    +{% block content %}
    +    {% if action_links %}
    +        <nav class="action-links">{{ action_links }}</nav>
    +    {% endif %}
    +{% endblock %}

    +++ b/core/modules/system/templates/block--system-page-title-block.html.twig
    @@ -0,0 +1,23 @@
    +{% block content %}
    +    {{ title_prefix }}
    +    {% if title %}
    +        <h1>{{ title }}</h1>
    +    {% endif %}
    +    {{ title_suffix }}
    +{% endblock %}

    That looks double indented (same with the other twig files)

mdrummond’s picture

StatusFileSize
new24.58 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,107 pass(es), 0 fail(s), and 1 exception(s).
[ View ]
new21.3 KB

I have drafts of title, actions, messages and tabs in here now. Tabs is the only one that seems to be working though. I'm pretty confused on how to get messages to work properly. Advice welcome.

Status:Needs review» Needs work

The last submitted patch, 39: 507488-39-convert-page-elements.patch, failed testing.

mdrummond’s picture

StatusFileSize
new23.49 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,982 pass(es), 0 fail(s), and 1 exception(s).
[ View ]
new7.51 KB

Cleaning up a few bugs. Messages is still wrong I'm sure. Title and actions don't work. Not sure why. Tabs is good to go.

mdrummond’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 41: 507488-41-convert-page-elements.patch, failed testing.

mdrummond’s picture

StatusFileSize
new22.8 KB
new4.21 KB

Removed the route object from the block constructors, and now the page title and page actions blocks work. So it's just messages that isn't functional yet.

mdrummond’s picture

Status:Needs work» Needs review
StatusFileSize
new24.94 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,134 pass(es).
[ View ]
new8.28 KB

Forgot to run testbot on 44, but I have a new version here that gets messages to work. Whether or not messages is working correctly, I don't know. Not sure if I need to do something special to have it pre render at the correct time, but it does seem to show messages.

One thing about the messages block: it can be instantiated, so that you could have more than one messages block in a layout. However, the messages will actually only show up in the first block that they appear in, because of how messages work (I think). It's possible that could be confusing, but I'm not sure what to do about that.

Anyhow, all of these blocks work now, which is pretty neat.

mdrummond’s picture

StatusFileSize
new26.68 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,402 pass(es), 117 fail(s), and 21 exception(s).
[ View ]
new6.41 KB

Status:Needs review» Needs work

The last submitted patch, 46: 507488-46-convert-page-elements.patch, failed testing.

mdrummond’s picture

Status:Needs work» Needs review
StatusFileSize
new26.68 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,376 pass(es), 121 fail(s), and 21 exception(s).
[ View ]
new6.41 KB

Bad patch. Hopefully this works.

Trying to get messages to fire at the right time. This isn't working yet. Just trying to debug.

mdrummond’s picture

StatusFileSize
new26.57 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,519 pass(es).
[ View ]

I turned off something that was making messages persist from page to page. We were using this while trying to get this to work, but it broke a lot of tests.

The last submitted patch, 48: 507488-46-convert-page-elements.patch, failed testing.

chx’s picture

StatusFileSize
new6.26 KB
new27.32 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,513 pass(es).
[ View ]

First thing visible, the getMessages method is not #markup but #message_list.

After that it's time to debug! Put a breakpoint in drupal_render $elements['#children'] .= drupal_render($elements[$key], TRUE); with a condition of $key === 'pagemessages' then step it. After some stepping you will arrive into _theme and more stepping will land you inside a template file that makes no sense. This is core/modules/system/templates/block--system-page-messages-block.html.twig which in #46 was a copy of the status messages twig template. But we need the block template not the status message template! It's not clear to me why the blocks need their own template -- why don't they just fall back to block.html.twig?? I don't get it. So I have cp core/modules/block/templates/block.html.twig core/modules/system/templates/block--system-page-messages-block.html.twig. However, the problem becomes that template_preprocess_status_messages() runs before our #pre_render -- which is good actually because we really wanted to run as late as possible. So the messages block won't operate until the old one is removed. This would require a block placement of the messages block in every core profile and the removal of template_preprocess_status_messages and the relevant array in template_preprocess_page as well. However, this patch intends to not do that for now so I have opted to use a new theme hook called status_messages_block using the same template. Left a @todo for later. Commenting out messages in template_preprocess_page shows this works.

Interdiff is against #46.

chx’s picture

StatusFileSize
new24.51 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,499 pass(es).
[ View ]
new2.98 KB

Oh it's not falling back to block.html.twig because it is told not to. Removed.

mdrummond’s picture

StatusFileSize
new24.94 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,499 pass(es).
[ View ]
new6.27 KB

Rolling a new version. This is an attempt to still use the message blocks. The markup varies between Stark and Bartik, which is why we can't just use the standard block template.

mdrummond’s picture

Trying another approach here. This is a variation on 52 that handles an exception with Bartik. Messages had a couple extra wrapper divs in the page template: this sets up a copy of the status-messages template in Bartik that can include those wrapper divs, then removes the wrapper divs from Bartik.

The problem with this approach is that the page messages block itself has the standard block wrapper divs, and those will show up regardless if there are any messages showing up in the block.

To compound that, messages will only show up once on the page. If a block is placed in the markup after the standard location where messages appear, then no messages will appear in that block. If there are multiple blocks, messages will appear in only the first block, if they appear at all.

There's not a great way to handle this. You could set drupal_get_messages(FALSE) throughout core, which prevents removing messages from the queue. But that prevents any removal ever, so messages would just stack up for all time. There's no way to just go ahead and remove the messages once all the blocks have rendered, at least not one that I can think of off hand.

Ideally we'd get a template working for the messages block, as we have done for the other page element blocks, as we could drop the standard block wrapping divs for the messages block. That would at least mean there wouldn't be a weird empty block appearing even if there are no messages.

mdrummond’s picture

StatusFileSize
new24.82 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,499 pass(es).
[ View ]
new4.37 KB

Ugh. Here's the actual patch.

mdrummond’s picture

StatusFileSize
new25.06 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,517 pass(es).
[ View ]
new1.02 KB

This version saves the messages as a static property on the messages block class. That allows messages to appear in multiple blocks with a couple caveats.

1) Messages will only appear in blocks if at least one block appears before where the standard messages show up in the page template.
2) If messages do appear in blocks, then they won't appear in the page template.

That's pretty confusing. So if we want to use this approach (and I think we probably should), then we'd probably want to remove the messages variable from the page template. That's on the to-do list anyhow, so this just moves that up a step.

I'd also need to take care of adding a messages block into a region with the install profile. That only happens at install, though, so making that change would remove messages from all existing d8 sites unless they do a reinstall or place a messages block.

If that seems problematic, we could split messages off into a separate issue so we could get the rest of these blocks in.

mdrummond’s picture

I should note that the static property idea was all chx's on IRC. He's been a huge huge help on this issue.

mdrummond’s picture

StatusFileSize
new26.03 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,520 pass(es).
[ View ]
new2.92 KB

So now I've been able to set up a block template that doesn't have wrapper divs. That prevents empty blocks from showing up when there aren't any messages.

It took some doing to find a way to properly detect if there are any messages or not in order to not show Bartik's specific message-wrapping divs, but I finally got it.

After discussion tonight, the next step will be to strip the messages from the page template by default and set up a region and default block for messages in the install profile.

mdrummond’s picture

StatusFileSize
new26 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,517 pass(es).
[ View ]

Didn't have a newline at the end of Bartik's status messages template. That's the only thing different here.

andypost’s picture

Issue tags:+Needs tests
StatusFileSize
new18.14 KB

The problem here is a useless tremendous big cache settings:

Suppose we should remove this details in favor of this message

+++ b/core/modules/system/src/Plugin/Block/SystemPageActionsBlock.php
@@ -0,0 +1,134 @@
+  public function buildConfigurationForm(array $form, array &$form_state) {
+    $form = parent::buildConfigurationForm($form, $form_state);
+
+    // The page actions block is never cacheable, because it may be dynamic.
+    $form['cache']['#disabled'] = TRUE;
+    $form['cache']['#description'] = $this->t('This block is never cacheable, it is not configurable.');
+    $form['cache']['max_age']['#value'] = 0;
+
+    return $form;

+++ b/core/modules/system/src/Plugin/Block/SystemPageMessagesBlock.php
@@ -0,0 +1,133 @@
+  public function buildConfigurationForm(array $form, array &$form_state) {
+    $form = parent::buildConfigurationForm($form, $form_state);
+
+    // The page messages block is never cacheable, because it may be dynamic.
+    $form['cache']['#disabled'] = TRUE;
+    $form['cache']['#description'] = $this->t('This block is never cacheable, it is not configurable.');
+    $form['cache']['max_age']['#value'] = 0;
+
+    return $form;

+++ b/core/modules/system/src/Plugin/Block/SystemPageTabsBlock.php
@@ -0,0 +1,100 @@
+  public function buildConfigurationForm(array $form, array &$form_state) {
+    $form = parent::buildConfigurationForm($form, $form_state);
+
+    // The page tabs block is never cacheable, because it may be dynamic.
+    $form['cache']['#disabled'] = TRUE;
+    $form['cache']['#description'] = $this->t('This block is never cacheable, it is not configurable.');
+    $form['cache']['max_age']['#value'] = 0;
+
+    return $form;

+++ b/core/modules/system/src/Plugin/Block/SystemPageTitleBlock.php
@@ -0,0 +1,135 @@
+  public function buildConfigurationForm(array $form, array &$form_state) {
+    $form = parent::buildConfigurationForm($form, $form_state);
+
+    // The page title block is never cacheable, because it may be dynamic.
+    $form['cache']['#disabled'] = TRUE;
+    $form['cache']['#description'] = $this->t('This block is never cacheable, it is not configurable.');
+    $form['cache']['max_age']['#value'] = 0;
+
+    return $form;

There's a lot of code duplication, seems better to make base class for that

mdrummond’s picture

Yes, quite possible I have the block caching settings wrong. I used the main content block as an example, and maybe that's the wrong way to go. I asked wimleers to take a look.

Not sure if an extra class is needed for these. There is some variation between them in their properties and constructors, so not sure if it's worth that. Let's figure out the the proper caching for these first.

Thanks for the feedback!

chx’s picture

Edit: nevermind. I am out of this issue too.

Wim Leers’s picture

StatusFileSize
new26.32 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,550 pass(es).
[ View ]
new5.68 KB

The big thing that I'm still missing in this patch, is the removal of the current way of rendering the page title, actions, tasks, etc., in favor of the blocks.


mdrummond pinged me regarding the cacheability of these four new blocks, and I wanted to enable caching for several of them. But… they actually all are uncacheable:

  1. SystemPageMessagesBlock: messages are per-session, and "expire" after a single page load
  2. SystemPageTitleBlock: the page title can depend on arbitrary logic (thanks to $route_title = call_user_func_array($callable, $arguments); in TitleResolver), until that returns cacheability metadata, that will be impossible to cache
  3. SystemPageActionsBlock and SystemPageTabsBlock are uncacheable for the same reason: what's visible depends on access checkers, and hence this is unfortunately uncacheable also, because that too may have arbitrary logic. Once #2287071: Add cacheability metadata to access checks lands though, those two will become cacheable!

So, I just updated the cacheability-related docs a bit, and that's all I was able to do, unfortunately.


Thanks for working on this, mdrummond — can't wait to get rid of all the special snowflake stuff :)

mdrummond’s picture

StatusFileSize
new34.07 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,189 pass(es), 1,394 fail(s), and 21 exception(s).
[ View ]
new8.15 KB

This patch should break lots of tests.

This removes messages from templates and adds it in as a block to a new messages region.

This does not have:
- Fixes for tests
- Fixes for any potential CSS issues
- Removing of the preprocess stuff that adds status_messages variable to the page template.

Wanted to see what tests break first, then go from there.

We definitely need to remove messages from templates because of the way that the queue gets cleared. For title, tabs and actions, those could probably be in a followup.

Status:Needs review» Needs work

The last submitted patch, 64: 507488-64-convert-page-elements.patch, failed testing.

mdrummond’s picture

Status:Needs work» Needs review
StatusFileSize
new34.86 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,594 pass(es), 27 fail(s), and 0 exception(s).
[ View ]
new3.04 KB

This should greatly reduce the test errors if not eliminate them.

The messages variable is back in the page templates for system, but still gone for Bartik and Seven.

I kept the messages region in the page templates for system, so that if anybody wants to post a messages block in a theme based on Stark, they have a handy region to place it in. That region would come before the messages variable, which I think would make more sense, as then people can tweak a block in the messages region however they like, and those changes would show up, whereas if the messages region came after the messages variable, it might be more confusing. Just trying to think about what happens if somebody copies the blocks in Bartik or Seven into a new theme based on system/Stark. Having the messages region there by default makes it easier for that to work as might be expected.

We'll see if this patch works better. 1,394 errors was not bad for a night's work.

mdrummond’s picture

Just to further explain, the reason for all the test failures was because those tests relied upon asserting text on the page that was in a status message. Most tests are run on the testing profile, which doesn't even have blocks as a dependency. So... having messages in the standard page template avoids that total test meltdown.

Status:Needs review» Needs work

The last submitted patch, 66: 507488-66-convert-page-elements.patch, failed testing.

mdrummond’s picture

27 failures is much more manageable. Will likely track those down this weekend.

xjm’s picture

So @mdrummond and I had a long conversation about this issue in IRC, which I'll try to summarize. @mdrummond is working here from the perspective that something that causes 1394 tests to fail (in this case removing the messages variable from the page template) is not acceptable. The reason for these test failures is that when the messages are only available as a block, and block module is not installed (as is the case in the testing profile), the messages no longer appear anywhere. So @mdrummond inferred from this that we should not remove the message from system's base page template -- only from the specific themes' templates. That way, the system messages show up via the page template in Stark.

The point I tried to make is that simply leaving the variable in the system page template to avoid test failures, and not as a deliberate technical choice, is incorrect. Either:

  1. We decide that system messages should always show up, even when the block module is not installed, and implement a solution to support that, or
  2. We decide that system messages only show up when the block module is available, and make the needed changes to tests to reflect that.

In case 1, leaving the messages in the base system page template may or may not be the correct solution. I'm not offering an opinion there. But in case 2, what we would need to do is go through those 1394 test assertions and update them. Not by adding block to the testing profile. Instead, we would:

  • Go through each test individually.
  • Decide what the intention of testing the system message is.
  • If the goal is actually to make sure a specific message is displayed, then we simply install block module in that particular test.
  • If the goal is instead to (e.g.) confirm that a form submission worked, or if the message was simply being used to display other test output, change the test to test that thing in a different way.

Now, I acknowledge that this is a big undertaking. So even if case 2 is what we agree on, then I could see leaving the messages in the base page template for now, and filing a followup issue to fix all those tests. Or we might decide that it's just too big of a change at this point in the release cycle. The point is, though, that we should make the decision for the right reason, and be explicit that we are making a technical or strategic decision, not just as a quicker way to avoid test failures. :) Thanks @mdrummond for your patience discussing this and your work here.

mdrummond’s picture

There are a lot of tests in core that check the value of a status message that appears on a page in order to determine if the test as a whole or that part of the test succeeds or fails.

Many of those tests are based on the assumption that there are no modules enabled for core.

So if we were to rely solely upon messages in blocks, we would need to go through all of the tests that assume there are no modules and are still trying to check the value of a status message that appears on the page.

It's possible that some of those tests could be reworked to still test what they're testing without relying upon a message appearing on a page to determine if that test passes or fails.

I'm concerned about that approach, because it could require a huge amount of work across a wide variety of systems throughout Drupal.

Maybe that means it's unrealistic to remove messages from the page template because of the incredible amount of work that would be required.

The reason we were looking at removing messages from the page template was because it caused a confusing problem. When messages are processed for the page template, the queue of messages is cleared.

So that means if somebody tried to place a block further down in the source code from where the messages variable appears, there would be no messages that appear in the block.

We previously solved the similar issue of messages only appearing in one block by having the messages block class store the processed messages in a static property on the messages block class, so that multiple blocks could make use of those messages.

That still leaves a potential issue where an unrelated block in a region further down the page from that initial block could generate a message, but it wouldn't appear until the next page, because messages had already been processed.

When messages are processed in the page template, that's near the top of the render stack, so there's a decent chance that all messages from that page will be processed. Although I think somebody tested and found that something in the page footer could generate a message, and it would only appear on the next page. So maybe it's a difference of degree rather than a difference of kind, between showing messages in blocks or in the page template.

I don't know. This was part of the old blocks everywhere issue. My understanding is that there was interest in having these page elements as blocks to be used in a Panels-type situation, and while Layout won't go into 8.0, there has been work restarted on a lot of different parts of layout work. If there's interest in making messages available in a block despite the complications, there might still be some ways we could do this.

For example, we could look at whether we really want drupal_get_messages to clear the messages queue at the moment it's called. Perhaps it could be set to not clear the messages by default, but then we could attach a post render callback to the html render element that clears the messages at basically the last possible moment. That also would allow any block on the page to display messages, no matter where it did so on the page.

The question is for the goals that are trying to be accomplished with the blocks everywhere initiative, is it important for messages to only appear within blocks? And for messages to not show up in the page template? Keeping in mind that that choice means that if there are no message blocks assigned to a page, it's possible for messages to stack up forever (if they're not cleared) or to just not get seen (if they're cleared on each page but not displayed in a block).

This is a difficult problem to solve. And if messages are removed from the page template, it's a LOT more difficult of a problem to solve.

I wonder if it might be worthwhile to split any potential messages block into a separate issue. That would allow the other items that are less problematic—page title, tabs and local actions—to go in now while messages is dealt with (or not dealt with).

The page title, tabs and local actions can be added as blocks without having to remove them from the page template. If somebody wants to put their page title in multiple places on the page, they could do so.

It's possible we'd find issues when we try to remove those items from the page template, but I think we could deal with that in a future page variable removal issue.

It should be noted that removing messages from the page template doesn't solve any issues with performance caching, as it needs to be called on every page load.

It's kicking the can down the road, but I think that splitting off messages is what I'd suggest.

webchick’s picture

I personally believe we should just always show messages, just as we should always show content. They can contain crucial debugging information and if you don't get it, you can't recover from an error condition you've inadvertently caused. For example, imagine a scenario where you were trying to add the "messages" block to your theme, but the placement of the block failed, but the reason for the failure couldn't be displayed because you have no messages block. :P~ Very, very annoying.

In other words, instead of only hard-coding $messages in a few themes' templates, we should instead just do it everywhere. Or whatever the special trick is that causes the "System content" block to show up even "minimal" profile.

xjm’s picture

An additional suggestion from @tim.plunkett is that block_preprocess_page() could remove the messages variable. That way, you always have to use blocks if blocks is enabled, and there's always a graceful fallback for when it isn't.

My one outstanding question for that solution is whether a preprocess function is too late for us to take full advantage of having messages in a separate thing caching-wise (so that we can cache everything but the messages, or asynchronously load messages into a cached page, or whatever). I left @Wim Leers a tell to help clarify this.

xjm’s picture

For example, imagine a scenario where you were trying to add the "messages" block to your theme, but the placement of the block failed, but the reason for the failure couldn't be displayed because you have no messages block. :P~ Very, very annoying.

Ah. That is also a good point...

mdrummond’s picture

Discussed this further with xjm and timplunkett on IRC.

Tim suggested that ideally we want to have the messages variable removed from the page template, but that it might be ok for it to be there for system/Stark. He suggested that one possible way to deal with this would be to have block_preprocess_page remove the messages variable. That way if blocks are disabled the messages variable shows up in the page template (at least in system/Stark). But if blocks are enabled then the status messages show up in a block.

That seems like a good way to solve the usability issues, as the page variable would no longer be fighting with the messages block.

xjm wondered if having the messages variable in the page template, but removed with block_preprocess_page, would still allow the page to be cached. The page would be mostly a collection of blocks, which could be cached or not on their own. Does the possibility of messages maybe being in the page template prevent the possibility of everything else from being cached?

We're hoping Wim Leers can answer that question.

mdrummond’s picture

xjm and I keep writing the same things at approximately the same time. :/

mdrummond’s picture

webchick: Yes, I had found how the mechanism that makes it impossible to disable content works. So that's possible to do.

mdrummond’s picture

webchick: Oh, also, fun fact. The main content block does *not* show up on the minimal profile.

chx’s picture

It seems to me that this issue is making a slowly brewing process to a point: blocks are required. Perhaps we want to split blocks UI off now that we actually have an API to deal with blocks so the UI is not mandatory. Even though SCOTCH failed, BOURBON is coming (you guys with your code names :D ) and perhaps makes it into core in 8.2 and then truly blocks everywhere. Even without that, surely page manager is the future even if it's in contrib.

andypost’s picture

The idea about post-render cache should work for messages.
But let's split the issue with messages out, please!
This block could provide a configuration to display only a limited types of messages,
so user can place error and info message block into different places ;)

Suppose messages are rendered last and this needs tests

+++ b/core/modules/system/src/Plugin/Block/SystemPageMessagesBlock.php
@@ -0,0 +1,134 @@
+      static::$messages = drupal_get_messages();

use drupal_render_cache_generate_placeholder() here

Wim Leers’s picture

I agree that the messages block should be handled in a different issue, because it's much trickier than the other blocks. We can easily get the majority of this patch done, and then focus on the messages block in a separate issue.

I agree that #post_render_cache is a near-ideal solution for this problem, because then it will indeed run at the very latest point.

I think the block_preprocess_page() think can work, but I can't know for certain without trying. This is similar to #2273277: Figure out a solution for the problematic interaction between the render system and the theme system when using #pre_render, in that it's extremely confusing to reason in your HEAD about the exact flow in which things are rendered, due to the awkward interactions between the theme and render system. The only way to answer this with confidence (or at least the only way *I* could answer it with confidence), is to step through it with a debugger.

chx’s picture

xjm’s picture

Let's file the separate issue for messages now as a postponed followup to this, so we can move relevant discussion there? Thanks everyone!

Wim Leers’s picture

I wrote HEAD where I meant head. Consequences of working on core, I guess :P (Also s/think/thing/.)

mdrummond’s picture

I'll open the new issue tonight or sometime this weekend and work on the next version for messages there, as well as splitting off the rest for here, so we can possibly get those in more quickly.

andypost’s picture

Filed #2289917: Convert "messages" page element into blocks
Patch needs re-roll without messages hunks

mdrummond’s picture

Title:Convert page elements (title, tabs, actions, messages) into blocks» Convert page elements (title, tabs, actions) into blocks
Status:Needs work» Needs review
Related issues:+#2289917: Convert "messages" page element into blocks
StatusFileSize
new18.49 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,734 pass(es).
[ View ]
new16.82 KB

Here is the patch rerolled to just have the title, tabs and actions blocks.

andrewko’s picture

Patch applies cleanly to current branch head. Noticed that the contextual links are not working on the page title block. It looks like the contextual links data attribute is being duplicated on the block itself and the H1 title suffix.

mdrummond’s picture

Not sure how to fix the page title quick links issue. Not sure if that would block us getting that in, or if it could be handled as a follow-up. If the former, maybe we should split page title off into a separate issue.

Probably will need to look at tabs and local actions again now that the new menu system is going in, to make sure all still works correctly.

Wim Leers’s picture

StatusFileSize
new18.62 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,168 pass(es).
[ View ]
new3.83 KB

It looks like the contextual links data attribute is being duplicated on the block itself and the H1 title suffix.

Confirmed. mdrummond asked me to do some debugging to find the cause.

Root cause: the new block--system-page-title-block.html.twig block was using {{ title_prefix }} and {{ title_suffix }} as if those are the page title prefix/suffix, but they're the block title prefix/suffix. Hence the same suffix is added twice. Hence this problem.

This will either need to do the work to get the actual page title prefix/suffix and pass those to the template (probably as {{ page_title_prefix }} instead of {{ title_prefix }}, and analogously for the suffix.

Finally, while doing this debugging (and after rerolling for HEAD), I noticed that the page title is wrong sometimes: in HEAD, the page title can be overridden by setting $page['#title']. E.g. in-place editing actively uses this capability to allow the title to be in-place edited. I'm afraid this will have to find a solution for that problem as well, and I don't know if accessing $page is at all possible… in theory it should be possible though (see drupal_prepare_page() and drupal_set_page_content(); drupal_prepare_page() invokes hook_page_build(), which includes block_page_build(), which builds all blocks).


P.S.: can any of you link me to the documentation on where the "Twig blocks" stuff is coming from? I'm not finding any useful docs and I hadn't seen that before:

    {% block content %}
      {{ content }}
    {% endblock %}

No idea what that means/does.

mdrummond’s picture

Twig blocks like the one you cited above allow you to create a child template that only replaces what is between the start of a Twig block and the endblock. The child template can then focus on only what's different in that specific area of the master template, and if the wrapping content in the master template, the child template automatically inherits those changes.

We first used this technique in the system branding block. #1053648: Convert site elements (site name, slogan, site logo) into blocks

edit: fixing the link to system branding block

dawehner’s picture

Status:Needs review» Needs work
  1. +++ b/core/modules/system/src/Plugin/Block/SystemPageActionsBlock.php
    @@ -0,0 +1,137 @@
    +    $links = menu_local_tasks();
    +    $request = $this->requestStack->getCurrentRequest();
    +    $route_name = $request->attributes->get(RouteObjectInterface::ROUTE_NAME);
    +    $action_links = $this->localActionManager->getActionsForRoute($route_name) + $links['actions'];

    Can we make a follow up to move the dynamic parts of menu_local_tasks() into the local action/task manager itself?

  2. +++ b/core/modules/system/src/Plugin/Block/SystemPageTitleBlock.php
    @@ -0,0 +1,136 @@
    +    $title = '';
    +    $request = $this->requestStack->getCurrentRequest();
    +    if ($route = $request->attributes->get(routeObjectInterface::ROUTE_OBJECT)) {
    +      $title = $this->titleResolver->getTitle($request, $route);
    +    }
    +

    I am not convinced that this is the right approach. The title is part of the HtmlPage and is collected there. The title resolver just allows you to get the title from the current route, which might be a different one.

mdrummond’s picture

Time to revive this. Now that post render callbacks should be working correctly, we should be able to get this working.

eiriksm’s picture

StatusFileSize
new18.27 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 507488-94-convert-page-elements-blocks.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

This is just a reroll with some minor changes of #90. Not sure an interdiff would be pretty looking, since the hook_theme part of it had to be applied manually. So just a small list:

- use Drupal\Core\Block\BlockBase; instead of Drupal\block\BlockBase;
- return FALSE on block build in cases where we have no content (so we don't output an empty block).

Setting to needs review anyway :)

eiriksm’s picture

Status:Needs work» Needs review

Yeah, and then really setting it to needs review.

joelpittet’s picture

One thing that stands out here is that those templates are quite small and single purpose. They feel a bit too granular to be their own templates.

To me these feel more like good cases for a token style replacement in a custom block. Unfortunately I don't have an example to point to for this:(

I do know these blocks can be useful, I use similar pieces with panels everywhere in drupal 7.

The granularity here worries me for performance issues and maintaining micro-templates.

Maintaining these little templates as a themer can be a bit dissociated with the page they are being put onto. Though as a site builder, dragging these around into different regions can be helpful.

Drupal 7 allows this some what with panels or/and tokens, I wonder if something could be accomplished here without the need for the small templates?

mdrummond’s picture

Ironically the templates are small because we're using twig blocks so that the templates are more DRY. ;)

If there's not an example of the sort of token-style approach you'd like to see, not sure we want to hold this up on that. Would you be okay if we looked at other options in a follow-up?

This is part of a larger issue of converting all page template variables to blocks, so getting all of these conversions will make it much easier to explain that everything at the page level is a block rather than a variable.

I'm guessing we still need to look at the issues in #90 and #92?

eiriksm’s picture

Yeah. I tried to find some info on how to get the title from HtmlPage, but the patch is more or less a reroll of #90. If anyone can point me in the right direction I'll try to get that in. If that is the right direction?

joelpittet’s picture

@mdrummond nm, just scared of change and kind of hypocritical of me because I do similar things with panels everywhere... carry on...

I do hope some of those blocks just become specialized menu blocks though for the tasks/tabs etc...

The title one worried me the most because we aren't blockifying the block titles, why blockify the page titles... but I'm sure there is a good use cases, just worry we don't abstract things so far that themers can't find where to change what.

Wim Leers’s picture

From #1947536-137: Convert drupal_get_breadcrumb() and drupal_set_breadcrumb() to a service by Tim Plunkett in May 2013:

Okay, so this works great, except we have no where to put the new breadcrumb block.
That should be left to #507488: Convert page elements (local tasks, actions) into blocks to handle.

If that's true, we should do it here, otherwise we'll have to open a follow-up.

Wim Leers’s picture

Issue tags:+D8 cacheability
Jeff Burnz’s picture

@101 - there is a follow up to other blocky issues #2005546: Use branding block in place of page template branding variables (site name, slogan, site logo), we could add it there (a Bartik issue), from what I can tell at a quick glance at the current bartik page template it would need at least one more region (for this patch, it also needs another for messages block when that lands), as for Seven I don't know how everyone wants to handle it, because it's the admin theme maybe it needs to build its own variables in preprocess and use those rather than blocks (it does this with primary_local_tasks and secondary_local_tasks).

Wim Leers’s picture

Title:Convert page elements (title, tabs, actions) into blocks» Convert page elements (title, tabs, actions, breadcrumbs) into blocks
EclipseGc’s picture

#102 sounds an awful lot like asking for layouts in core.

mdrummond’s picture

This isn't full-on layouts in core, just the blocks everywhere part.

Wim Leers’s picture

Exactly!

Status:Needs review» Needs work

The last submitted patch, 94: 507488-94-convert-page-elements-blocks.patch, failed testing.

Jeff Burnz’s picture

Title:Convert page elements (title, tabs, actions, breadcrumbs) into blocks» Convert page elements (title, tabs, actions) into blocks

As we know breaedcrumb is a block already so just updating title.

Do we need the justification for beta inclusion for this task? I assume not since this has always been part of the overall plan of having everything as blocks.

Wim Leers’s picture

#109: but it's not actually used yet… That's why I added it. See our discussion in #100/#102. If you want to do it elsewhere, that's fine, but currently it's being forgotten.

Jeff Burnz’s picture

Sorry Wim, I missed that, the issue afaict is being addressed here: #2306407: Remove breadcrumb from page template

Its all getting a bit messy really, I putting together an overview for myself of all these issues to get some perspective on exactly what needs to be done with all these conversion issues.

Wim Leers’s picture

Jeff Burnz++

Jeff Burnz’s picture

+++ b/core/modules/system/src/Plugin/Block/SystemPageTabsBlock.php
@@ -0,0 +1,103 @@
+    // @todo Make cacheable once https://drupal.org/node/2287071 lands.

Has landed, what do we need to do here?

I can get a strait re-roll mostly working except for the Actions block, assumed something changed here but I have no real clue what.

The page title block appears to have some issues (#92?), e.g. does not appear on admin/content or node edit pages, different on pages such as node/1/outline.

Wim Leers’s picture

Now that access results include cacheability metadata, it's now possible for that todo to be replaced with logic that indicates whether the tab on the current page are cacheable or not. It needs to be cached per URL (since the tabs on each URL may be different), so cache_context.url should be used as a cache context, and then the cacheability metadata for the access checks associated with the routes associated with the tabs need to be used.

mdrummond’s picture

So just to recap, we need a reroll, plus addressing concerns in #90 and #92, and we may need to include breadcrumbs as well?

mdrummond’s picture

Never mind the last part, now I see that is being taken care of in #2306407: Remove breadcrumb from page template.

Jeff Burnz’s picture

And titles, e.g. in View pages the current method does not work, I'm not entirely sure how we are meant to retrieve a title for views, I looked in the Views module and tested with the following, however I'm not sure if there is an overall better way of doing the titles:

<?php
   
if (empty($title)) {
      if (
is_callable('views_get_page_view')) {
        if (
$view = views_get_page_view()) {
         
$title = $view->getTitle();
        }
      }
    }
?>
Manuel Garcia’s picture

Status:Needs work» Needs review
StatusFileSize
new18.27 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/modules/system/src/Block/SystemPageActionsBlock.php.
[ View ]

Straight reroll of #94.

Status:Needs review» Needs work

The last submitted patch, 118: convert-page-elements-blocks-507488-118.patch, failed testing.

Manuel Garcia’s picture

Status:Needs work» Needs review
StatusFileSize
new18.27 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/modules/system/src/Block/SystemPageActionsBlock.php.
[ View ]

Oops... here we go again.

BTW, for Banana phase 2, this patch currently adds these new twig files to system:

  • core/modules/system/templates/block--system-page-actions-block.html.twig
  • core/modules/system/templates/block--system-page-tabs-block.html.twig
  • core/modules/system/templates/block--system-page-title-block.html.twig

Status:Needs review» Needs work

The last submitted patch, 120: convert-page-elements-blocks-507488-120.patch, failed testing.

mortendk’s picture

Issue tags:+classy
Manuel Garcia’s picture

Status:Needs work» Needs review
StatusFileSize
new18.28 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,297 pass(es).
[ View ]

Of course if you fix the error, but don't save the file.... sigh

tim.plunkett’s picture

Status:Needs review» Needs work
  1. +++ b/core/modules/system/src/Block/SystemPageActionsBlock.php
    @@ -0,0 +1,140 @@
    +    $build = array();

    Might as well use [] these days (here and throughout the patch)

  2. +++ b/core/modules/system/src/Block/SystemPageActionsBlock.php
    @@ -0,0 +1,140 @@
    +    $links = menu_local_tasks();

    Please wrap this function in a protected method

  3. +++ b/core/modules/system/src/Block/SystemPageActionsBlock.php
    @@ -0,0 +1,140 @@
    +    $request = $this->requestStack->getCurrentRequest();
    +    $route_name = $request->attributes->get(RouteObjectInterface::ROUTE_NAME);

    +++ b/core/modules/system/src/Block/SystemPageTitleBlock.php
    @@ -0,0 +1,139 @@
    +    $request = $this->requestStack->getCurrentRequest();
    +    if ($route = $request->attributes->get(routeObjectInterface::ROUTE_OBJECT)) {

    Instead, use the route_match service

  4. +++ b/core/modules/system/src/Block/SystemPageActionsBlock.php
    @@ -0,0 +1,140 @@
    +    // The "Page actions" block is never cacheable, due to access checking.
    +    $form['cache']['#disabled'] = TRUE;
    +    $form['cache']['#description'] = $this->t('This block is never cacheable because access checking is needed, it is not configurable.');
    +    $form['cache']['max_age']['#value'] = 0;
    ...
    +    // The "Page Actions" block is never cacheable, because its contents depends
    +    // on access checks, which are currently uncacheable.
    +    // @todo Make cacheable once https://drupal.org/node/2287071 lands.
    +    return FALSE;

    +++ b/core/modules/system/src/Block/SystemPageTabsBlock.php
    @@ -0,0 +1,107 @@
    +    // The "Page tabs" block is never cacheable, due to access checking.
    +    $form['cache']['#disabled'] = TRUE;
    +    $form['cache']['#description'] = $this->t('This block is never cacheable because access checking is needed, it is not configurable.');
    +    $form['cache']['max_age']['#value'] = 0;
    ...
    +    // The "Page Tabs" block is never cacheable, because its contents depends
    +    // on access checks, which are currently uncacheable.
    +    // @todo Make cacheable once https://drupal.org/node/2287071 lands.
    +    return FALSE;

    #2287071: Add cacheability metadata to access checks went in last September.

  5. +++ b/core/modules/system/src/Block/SystemPageTabsBlock.php
    @@ -0,0 +1,107 @@
    +    $tabs = menu_local_tabs();
    ...
    +    $build['tabs']['#markup'] = menu_local_tabs();

    Also wrap this function in a method, and don't call it twice

Manuel Garcia’s picture

Status:Needs work» Needs review
StatusFileSize
new18.17 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,794 pass(es).
[ View ]
new2.36 KB

Attached patch fixes point 1 from #124.

I cant see the blocks showing up on /admin/structure/block page though :/

At the moment the patch only introduces the new blocks, so there's plenty to be done here still besides what's mentioned on #124.

Manuel Garcia’s picture

StatusFileSize
new29.5 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,938 pass(es), 516 fail(s), and 491 exception(s).
[ View ]
new11.33 KB

Some notes on this patch:

  • We should figure out what to do with title_prefix and title_suffix page variables, since the page title is a block with this patch.
  • I did not see tabs variable being printed on page template for seven, so I did not add the region to it.
  • We still need to do what was mentioned on #124.

Done:

  • Removed title, action_links and tabs variables from template_preprocess_page.
  • Added the regions to ThemeHandler::rebuildThemeData().
  • Modified bartik, classy and seven themes adding the new regions and swapping out the variables for the regions' content.

Test bot should start spitting out failing tests now.

Status:Needs review» Needs work

The last submitted patch, 126: convert-page-elements-blocks-507488-126.patch, failed testing.

Jeff Burnz’s picture

+++ b/core/modules/system/templates/page.html.twig
@@ -52,6 +47,9 @@
+ * - page.title: Items for the page title region.
+ * - page.tabs: Items for the tabs region.
+ * - page.actions: Items for the actions region.

Can't say for Seven, but I don't think we need new regions for these blocks, these I think can all go in main content region, except as mentioned for Seven which Lewis will have to chime in on. I'd really hate to see us put in regions specifically for a block like this.

Manuel Garcia’s picture

@Jeff Burnz I tend to agree with you.. I have done it like this just to follow what's been done for breadcrumbs etc. so far.

We could discuss whether to do as you suggest, or to do so in a follow up, cleaning up unnecessary regions - which would take longer I suppose...

mortendk’s picture

@jeff +1 on that, maybe thats a followup issue, with breadcrumb we kinda pulled the tricker to get it cleaned out - i guess it would make sense to go back & do some housecleaninng on all regions on the page, maybe a follow up :)

Manuel Garcia’s picture

Status:Needs work» Needs review
StatusFileSize
new33.47 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,701 pass(es), 542 fail(s), and 494 exception(s).
[ View ]
new5.75 KB

Some progress on this:

  • Moved the new Block plugins into the proper directory.
  • Added the tabs region to seven which was missing before.
  • Added the standard install profile files for title, tabs and actions for both bartik and seven themes.

Status:Needs review» Needs work

The last submitted patch, 131: convert-page-elements-blocks-507488-131.patch, failed testing.

Manuel Garcia’s picture

Status:Needs work» Needs review
StatusFileSize
new35.85 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,752 pass(es), 563 fail(s), and 330 exception(s).
[ View ]
new3.49 KB

Some more cleanup:

  • Seven theme: Getting rid of primary_local_tasks and secondary_local_tasks since we no longer have a tabs variable.
  • Bartik theme: Removing title_prefix and title_suffix since we no longer have a title var, so they will always be empty in twig. If we still want these we should probably implement them as part of the title block.

Status:Needs review» Needs work

The last submitted patch, 133: convert-page-elements-blocks-507488-132.patch, failed testing.

Manuel Garcia’s picture

Point 2 on #92 is something I need help with... I am seeing different titles on some routes (like when you delete a block from the manage blocks page). Fixing this will fix some of the failing tests as well.

Manuel Garcia’s picture

Forgot to mention before that the blocks now show up on the admin interface properly, and that they are assigned, and working upon install on both seven and bartik. The title block still needs to be looked at as per my previous comment.

Wim Leers’s picture

Status:Needs work» Needs review
StatusFileSize
new43.23 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,809 pass(es), 517 fail(s), and 328 exception(s).
[ View ]
new12.02 KB

#135/ #92.2: the problem is that it's in rare cases in fact necessary to let the content associated with a route generate the title.

From HtmlRenderer::prepare():

    if (…) {
      …

      // We must render the main content now already, because it might provide a
      // title. We set its $is_root_call parameter to FALSE, to ensure
      // #post_render_cache callbacks are not yet applied. This is essentially
      // "pre-rendering" the main content, the "full rendering" will happen in
      // ::renderResponse().
      // @todo Remove this once https://www.drupal.org/node/2359901 lands.
      if (!empty($main_content)) {
        $this->renderer->render($main_content, FALSE);
        $main_content = $this->renderer->getCacheableRenderArray($main_content) + [
          '#title' => isset($main_content['#title']) ? $main_content['#title'] : NULL
        ];
      }
    }

    …

    // Determine the title: use the title provided by the main content if any,
    // otherwise get it from the routing information.
    $title = isset($main_content['#title']) ? $main_content['#title'] : $this->titleResolver->getTitle($request, $route_match->getRouteObject());

    return [$page, $title];
  }

The only way to solve this is by doing something like we did in #2363025: Push usage of drupal_set_page_content() higher: out of blocks and page display variants to support a block containing the main content: we added MainContentBlockPluginInterface, now we also need TitleBlockPluginInterface.

Adding that.

Keep up the good work :)

Wim Leers’s picture

Title:Convert page elements (title, tabs, actions) into blocks» Convert page elements (title, local tasks, actions) into blocks

Status:Needs review» Needs work

The last submitted patch, 137: convert-page-elements-blocks-507488-137.patch, failed testing.

Fabianx’s picture

To fix tests properly we need to do the same I think for local tasks and actions as for title and messages to have fallback states ...

Do we really want 5 interfaces for that?

It seems the cleanest however ...

Wim Leers’s picture

Title:Convert page elements (title, local tasks, actions) into blocks» [PP-1] Convert page elements (title, local tasks, actions) into blocks
Status:Needs work» Postponed

Let's first get #2289917: Convert "messages" page element into blocks in, since that one is very close to RTBC and will conflict with this. Postponing this one.

Wim Leers’s picture

@manuee pointed me to #2005546: Use branding block in place of page template branding variables (site name, slogan, site logo), which hasn't gotten a comment in >4 months. We should finish that one too.

Wim Leers’s picture

Title:[PP-1] Convert page elements (title, local tasks, actions) into blocks» Convert page elements (title, local tasks, actions) into blocks
Status:Postponed» Needs work

#2289917: Convert "messages" page element into blocks landed, this is now unblocked!

I think it may be wiser to first tackle the page title, and then tackle actions & tabs. The page title is more complex, because it's very dynamic: it comes from the route by default, but may be overridden by the main content. Thoughts?

Fabianx’s picture

Yes, lets split off page title off this, so it can be independently reviewed.

Manuel Garcia’s picture

True the page title is its own little beast. +1 on working separately on it.

Manuel Garcia’s picture

Issue tags:+Needs reroll
Manuel Garcia’s picture

Title:Convert page elements (title, local tasks, actions) into blocks» Convert page elements (local tasks, actions) into blocks
Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new24.48 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/modules/system/src/Plugin/Block/SystemPageActionsBlock.php.
[ View ]

Here is the last patch #137 rerolled, but without the title stuff, so only tabs and actions.

Let's see what the bot has to say, hopefully I did not forget anything.

Next steps:

  • Take the patch #137 and reroll it with ONLY the title stuff, and post it on a new issue, yet to be created.
  • Get the attached patch to green.
Manuel Garcia’s picture

OK I've split the title part into its own patch, have a look here #2476947: Convert "title" page element into blocks

Let's continue to push this forward! -=]

Manuel Garcia’s picture

Status:Needs review» Needs work

The last submitted patch, 147: 507488-147-action-tabs-into-blocks.patch, failed testing.

joelpittet’s picture

@Manuel Garcia great idea, thank you for splitting and re-rolling.

Couple of items I spotted on a drive by review to help find that PHP error:

  1. +++ b/core/modules/system/src/Plugin/Block/SystemPageActionsBlock.php
    @@ -0,0 +1,136 @@
    +    $action_links = $this->localActionManager->getActionsForRoute($route_name)  $links['actions'];

    This looks like the culprit!

  2. +++ b/core/modules/system/system.module
    @@ -169,6 +169,14 @@ function system_theme() {
    +    'block__system_page_actions_block' => array(
    ...
    +    'block__system_page_tabs_block' => array(

    Nit: short array syntax.

  3. +++ b/core/modules/system/system.module
    @@ -778,6 +786,20 @@ function system_preprocess_block(&$variables) {
    +      if($variables['content']['action_links']['#markup']) {
    ...
    +      if($variables['content']['tabs']['#markup']) {

    Nit: Space after the 'if'.

Manuel Garcia’s picture

Status:Needs work» Needs review
StatusFileSize
new24.48 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]
new1.61 KB

Thanks a lot for the quick review joelpittet!!

About your point 2, I did not use short array syntax on system_theme because the rest of the implementations there are not using it... perhaps worthy of a cleanup issue, I just didn't want to create syntax noise there for now.

Fixed the fatal error (1) and your point 3.

I have noticed that with this patch, after running a clean install, the tabs don't show up until you've drush cr... very strange, but something we should look into. Setting this to needs review for the bot to chew on.

Wim Leers’s picture

Status:Needs review» Needs work

Thank you so much to get this going again! :)

  1. +++ b/core/modules/system/src/Plugin/Block/SystemPageActionsBlock.php
    @@ -0,0 +1,136 @@
    + * Provides a block to display the page local actions.
    ...
    + *   admin_label = @Translation("Page actions")

    Is it "Page actions" or "Page local actions"? Or just "Actions" or "Local actions"?

  2. +++ b/core/modules/system/src/Plugin/Block/SystemPageActionsBlock.php
    @@ -0,0 +1,136 @@
    +   * Stores the configuration factory.

    +++ b/core/modules/system/src/Plugin/Block/SystemPageTabsBlock.php
    @@ -0,0 +1,105 @@
    +   * Stores the configuration factory.

    s/Stores the/The/

  3. +++ b/core/modules/system/src/Plugin/Block/SystemPageActionsBlock.php
    @@ -0,0 +1,136 @@
    +   * A local action manger.

    The local action manager.

  4. +++ b/core/modules/system/src/Plugin/Block/SystemPageActionsBlock.php
    @@ -0,0 +1,136 @@
    +   * @var \Drupal\Core\Menu\LocalActionManager
    ...
    +   * @param \Drupal\Core\Menu\LocalActionManager $local_action_manager
    ...
    +  public function __construct(array $configuration, $plugin_id, $plugin_definition, ConfigFactoryInterface $config_factory, LocalActionManager $local_action_manager, RequestStack $request_stack) {

    Should typehint to the interface.

  5. +++ b/core/modules/system/src/Plugin/Block/SystemPageActionsBlock.php
    @@ -0,0 +1,136 @@
    +   *   The factory for configuration objects.

    +++ b/core/modules/system/src/Plugin/Block/SystemPageTabsBlock.php
    @@ -0,0 +1,105 @@
    +   *   The factory for configuration objects.

    Should match the above.

  6. +++ b/core/modules/system/src/Plugin/Block/SystemPageActionsBlock.php
    @@ -0,0 +1,136 @@
    +    $links = menu_local_tasks();
    +    $request = $this->requestStack->getCurrentRequest();
    +    $route_name = $request->attributes->get(RouteObjectInterface::ROUTE_NAME);
    +    $action_links = $this->localActionManager->getActionsForRoute($route_name) + $links['actions'];

    So this is the body of menu_get_local_actions().

    That means we should also remove that function from menu.inc.

  7. +++ b/core/modules/system/src/Plugin/Block/SystemPageActionsBlock.php
    @@ -0,0 +1,136 @@
    +    $build['action_links'] = ['#markup' => $action_links];

    $action_links is not markup, but a render array. Hence this assignment doesn't make sense. It should be:

    $build['action_links'] = $action_links;
  8. +++ b/core/modules/system/src/Plugin/Block/SystemPageActionsBlock.php
    @@ -0,0 +1,136 @@
    +  public function isCacheable() {
    +    // The "Page Actions" block is never cacheable, because its contents depends
    +    // on access checks, which are currently uncacheable.
    +    // @todo Make cacheable once https://drupal.org/node/2287071 lands.
    +    return FALSE;
    +  }

    ::isCacheable() doesn't exist anymore. What you want, is ::getCacheMaxAge().

    But… actually, this is now cacheable, because #2287071: Add cacheability metadata to access checks already landed.

    \Drupal\Core\Menu\LocalActionManager::getActionsForRoute() contains this:

    $links[$plugin_id] = array(
      …
      '#access' => $this->accessManager->checkNamedRoute($route_name, $route_parameters, $this->account),
      …
    );

    Instead, you want to change that to:

    $access = $this->accessManager->checkNamedRoute($route_name, $route_parameters, $this->account, TRUE);
    $links[$plugin_id] = array(
      …
      '#access' => $access->isAllowed(),
      …
    );
    \Drupal::service('renderer)->addDependency($links, $access);

    That will make sure the necessary cacheability metadata is present.

  9. +++ b/core/modules/system/src/Plugin/Block/SystemPageTabsBlock.php
    @@ -0,0 +1,105 @@
    +    $tabs = menu_local_tabs();
    ...
    +    $build['tabs']['#markup'] = menu_local_tabs();

    1) you call the same function twice.
    2) the assignment to #markup is wrong again.

  10. +++ b/core/modules/system/src/Plugin/Block/SystemPageTabsBlock.php
    @@ -0,0 +1,105 @@
    +  public function isCacheable() {
    +    // The "Page Tabs" block is never cacheable, because its contents depends
    +    // on access checks, which are currently uncacheable.
    +    // @todo Make cacheable once https://drupal.org/node/2287071 lands.
    +    return FALSE;
    +  }

    Similar to remarks above.

  11. +++ b/core/modules/system/system.module
    @@ -778,6 +786,20 @@ function system_preprocess_block(&$variables) {
    +      if ($variables['content']['action_links']['#markup']) {
    +        $variables['action_links'] = $variables['content']['action_links']['#markup'];
    ...
    +      if ($variables['content']['tabs']['#markup']) {
    +        $variables['tabs'] = $variables['content']['tabs']['#markup'];

    Ah, so this is why it worked at all: this was assigning the value of #markup. Just omit these now :)

  12. +++ b/core/modules/system/templates/block--system-page-actions-block.html.twig
    @@ -0,0 +1,18 @@
    +{% block content %}
    +  {% if action_links %}
    +    <nav class="action-links">{{ action_links }}</nav>
    +  {% endif %}
    +{% endblock %}

    +++ b/core/modules/system/templates/page.html.twig
    @@ -113,10 +111,10 @@
    +      {% if page.actions %}
    +        <nav class="action-links">{{ page.actions }}</nav>s
           {% endif %}

    Duplication. Should get the <nav> twice.

  13. +++ b/core/themes/bartik/templates/block--system-page-actions-block.html.twig
    @@ -0,0 +1,20 @@
    +    <ul class="action-links">

    +++ b/core/themes/bartik/templates/page.html.twig
    @@ -130,14 +128,14 @@
    +              <ul class="action-links">{{ page.actions }}</ul>

    Duplication again.

  14. +++ b/core/themes/bartik/templates/block--system-page-tabs-block.html.twig
    @@ -0,0 +1,20 @@
    +     <nav class="tabs" role="navigation" aria-label="{{ 'Tabs'|t }}">

    +++ b/core/themes/bartik/templates/page.html.twig
    @@ -130,14 +128,14 @@
                   <nav class="tabs" role="navigation" aria-label="{{ 'Tabs'|t }}">

    And again.

  15. +++ b/core/themes/classy/templates/layout/page.html.twig
    @@ -110,10 +108,10 @@
    +        <nav class="action-links">{{ page.actions }}</nav>

    Again.

The last submitted patch, 152: 507488-152-action-tabs-into-blocks.patch, failed testing.

joelpittet’s picture

@Manuel Garcia re #152 It's a tricky problem, we want short array syntax but creating issues for that causes other issues to unnecessarily re-roll. So the compromise to clean that up and move forward is to update diff hunks that have changed or added to the new short array syntax. That way it will move towards to goal without unnecessarily disrupting other patches needlessly.

Manuel Garcia’s picture

Status:Needs work» Needs review
StatusFileSize
new27.02 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
new11.52 KB

@Wim Leers Awesome thanks for the review! Here's some progress on your input:
1. Yep, that was confusing, I've updated the comment line to be coherent with the translation string.
Provides a block to display the page local actions. > Provides a block to display the page actions.

2. I'm not sure I get what you mean here, i did this...
Stores the configuration factory. > The configuration factory.

3. OK done

4. This is what you mean right? I am actually starting to understand OOP I think.

diff --git a/core/modules/system/src/Plugin/Block/SystemPageActionsBlock.php b/core/modules/system/src/Plugin/Block/SystemPageActionsBlock.php
index 3d5a9e0..e081288 100644
--- a/core/modules/system/src/Plugin/Block/SystemPageActionsBlock.php
+++ b/core/modules/system/src/Plugin/Block/SystemPageActionsBlock.php
@@ -10,7 +10,7 @@
use Drupal\Core\Block\BlockBase;
use Drupal\Core\Config\ConfigFactoryInterface;
use Drupal\Core\Form\FormStateInterface;
-use Drupal\Core\Menu\LocalActionManager;
+use Drupal\Core\Menu\LocalActionManagerInterface;
use Drupal\Core\Plugin\ContainerFactoryPluginInterface;
use Symfony\Cmf\Component\Routing\RouteObjectInterface;
use Symfony\Component\DependencyInjection\ContainerInterface;
@@ -36,7 +36,7 @@ class SystemPageActionsBlock extends BlockBase implements ContainerFactoryPlugin
   /**
    * A local action manger.
    *
-   * @var \Drupal\Core\Menu\LocalActionManager
+   * @var \Drupal\Core\Menu\LocalActionManagerInterface
    */
   protected $localActionManager;

@@ -58,12 +58,12 @@ class SystemPageActionsBlock extends BlockBase implements ContainerFactoryPlugin
    *   The plugin implementation definition.
    * @param \Drupal\Core\Config\ConfigFactoryInterface $config_factory
    *   The factory for configuration objects.
-   * @param \Drupal\Core\Menu\LocalActionManager $local_action_manager
+   * @param \Drupal\Core\Menu\LocalActionManagerInterface $local_action_manager
    *   The local action manager.
    * @param \Symfony\Component\HttpFoundation\RequestStack $request_stack
    *   The request stack object.
    */
-  public function __construct(array $configuration, $plugin_id, $plugin_definition, ConfigFactoryInterface $config_factory, LocalActionManager $local_action_manager, RequestStack $request_stack) {
+  public function __construct(array $configuration, $plugin_id, $plugin_definition, ConfigFactoryInterface $config_factory, LocalActionManagerInterface $local_action_manager, RequestStack $request_stack) {
     parent::__construct($configuration, $plugin_id, $plugin_definition);
     $this->configFactory = $config_factory;
     $this->localActionManager = $local_action_manager;

5. OK done, did it as well for the local action manager.

6. OK nuked.

7. Right, done.

8. This is a tricky one. I've done the following, hope it makes sense (this is all new to me):

diff --git a/core/lib/Drupal/Core/Menu/LocalActionManager.php b/core/lib/Drupal/Core/Menu/LocalActionManager.php
index 208075c..b2ea312 100644
--- a/core/lib/Drupal/Core/Menu/LocalActionManager.php
+++ b/core/lib/Drupal/Core/Menu/LocalActionManager.php
@@ -174,6 +174,7 @@ public function getActionsForRoute($route_appears) {
     foreach ($this->instances[$route_appears] as $plugin_id => $plugin) {
       $route_name = $plugin->getRouteName();
       $route_parameters = $plugin->getRouteParameters($this->routeMatch);
+      $access = $this->accessManager->checkNamedRoute($route_name, $route_parameters, $this->account, TRUE);
       $links[$plugin_id] = array(
         '#theme' => 'menu_local_action',
         '#link' => array(
@@ -181,9 +182,10 @@ public function getActionsForRoute($route_appears) {
           'url' => Url::fromRoute($route_name, $route_parameters),
           'localized_options' => $plugin->getOptions($this->routeMatch),
         ),
-        '#access' => $this->accessManager->checkNamedRoute($route_name, $route_parameters, $this->account),
+        '#access' => $access->isAllowed(),
         '#weight' => $plugin->getWeight(),
       );
+      \Drupal::service('renderer')->addDependency($links, $access);
     }
     return $links;
   }
diff --git a/core/modules/system/src/Plugin/Block/SystemPageActionsBlock.php b/core/modules/system/src/Plugin/Block/SystemPageActionsBlock.php
index ef29d50..44f7d0d 100644
--- a/core/modules/system/src/Plugin/Block/SystemPageActionsBlock.php
+++ b/core/modules/system/src/Plugin/Block/SystemPageActionsBlock.php
@@ -126,10 +126,8 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
   /**
    * {@inheritdoc}
    */
-  public function isCacheable() {
-    // The "Page Actions" block is never cacheable, because its contents depends
-    // on access checks, which are currently uncacheable.
-    // @todo Make cacheable once https://drupal.org/node/2287071 lands.
+  public function getCacheMaxAge() {
+    // @todo Make me cacheable now that https://drupal.org/node/2287071 has landed.
     return FALSE;
   }

9. Yup, done.

10. OK I did this, trying to follow what I did for point 8, hope it makes some sense:

diff --git a/core/lib/Drupal/Core/Menu/LocalTaskManager.php b/core/lib/Drupal/Core/Menu/LocalTaskManager.php
index 2ef5de1..afbd50d 100644
--- a/core/lib/Drupal/Core/Menu/LocalTaskManager.php
+++ b/core/lib/Drupal/Core/Menu/LocalTaskManager.php
@@ -305,8 +305,8 @@ public function getTasksBuild($current_route_name) {
         $route_parameters = $child->getRouteParameters($this->routeMatch);

         // Find out whether the user has access to the task.
-        $access = $this->accessManager->checkNamedRoute($route_name, $route_parameters, $this->account);
-        if ($access) {
+        $access = $this->accessManager->checkNamedRoute($route_name, $route_parameters, $this->account, TRUE);
+        if ($access->isAllowed()) {
           $active = $this->isRouteActive($current_route_name, $route_name, $route_parameters);

           // The plugin may have been set active in getLocalTasksForRoute() if
@@ -327,6 +327,7 @@ public function getTasksBuild($current_route_name) {
             '#access' => $access,
           );
         }
+        \Drupal::service('renderer')->addDependency($build, $access);
       }
     }
     return $build;
diff --git a/core/modules/system/src/Plugin/Block/SystemPageTabsBlock.php b/core/modules/system/src/Plugin/Block/SystemPageTabsBlock.php
index de54048..0ec09ba 100644
--- a/core/modules/system/src/Plugin/Block/SystemPageTabsBlock.php
+++ b/core/modules/system/src/Plugin/Block/SystemPageTabsBlock.php
@@ -95,10 +95,8 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
   /**
    * {@inheritdoc}
    */
-  public function isCacheable() {
-    // The "Page Tabs" block is never cacheable, because its contents depends
-    // on access checks, which are currently uncacheable.
-    // @todo Make cacheable once https://drupal.org/node/2287071 lands.
+  public function getCacheMaxAge() {
+    // @todo Make cacheable now that https://drupal.org/node/2287071 has landed.
     return FALSE;
   }


11.
Right! Done.

12, 13, 14, 15. Good catch, fixed.

@joelpittet ok fair enough, changed those to short array syntax.

Status:Needs review» Needs work

The last submitted patch, 156: 507488-156-action-tabs-into-blocks.patch, failed testing.

joelpittet’s picture

  1. +++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
    @@ -457,6 +457,8 @@ public function rebuildThemeData() {
    +        'page_tabs' => 'Page tabs',
    +        'page_actions' => 'Page actions',

    Isn't the region's named tabs and actions? This may be the failure?... just a guess

  2. +++ b/core/modules/system/system.module
    @@ -778,6 +786,20 @@ function system_preprocess_block(&$variables) {
    +      $variables['action_links'] = '';
    +      if ($variables['content']['action_links']) {
    +        $variables['action_links'] = $variables['content']['action_links'];
    +      }
    ...
    +      $variables['tabs'] = '';
    +      if ($variables['content']['tabs']) {
    +        $variables['tabs'] = $variables['content']['tabs'];
    +      }

    These should check isset() if they aren't always expected to exist because that will throw notices the way it's written OR if they are always expected, just set the action_links/tabs in one line because ''/NULL/array() will all print the same nothing in the template.

Manuel Garcia’s picture

Status:Needs work» Needs review
StatusFileSize
new26.83 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch convert_page_elements-507488-159.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new1.5 KB

Awesome @joelpittet thanks!

1. That was it. After fixing that the tabs appear on fresh installation just fine.
2. Makes sense, changed them to one liners.

Status:Needs review» Needs work

The last submitted patch, 159: convert_page_elements-507488-159.patch, failed testing.

xjm’s picture

Issue tags:+Needs beta evaluation

Thanks @Manuel Garcia and @joelpittet for keeping this issue going!

We should probably do a detailed beta evaluation on this.

Manuel Garcia’s picture

Further info, the tabs appear on the user/ID page, but not on the node/ID pages... cant figure out why.

The last submitted patch, 159: convert_page_elements-507488-159.patch, failed testing.

borisson_’s picture

StatusFileSize
new26.83 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

Rerolled the patch, there were conflicts in the following files:

- error: patch failed: core/themes/bartik/bartik.info.yml:24
- error: core/themes/bartik/bartik.info.yml: patch does not apply
- error: patch failed: core/themes/seven/templates/page.html.twig:64
- error: core/themes/seven/templates/page.html.twig: patch does not apply

borisson_’s picture

Status:Needs work» Needs review
bill richardson’s picture

Status:Needs review» Needs work

Applying patch to latest head - after enabling actions or tabs makes site unusable -- If patch applied before new installation , fails after last installation step.

The last submitted patch, 165: convert_page_elements-507488-165.patch, failed testing.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 165: convert_page_elements-507488-165.patch, failed testing.

lauriii’s picture

Assigned:Unassigned» lauriii
Status:Needs work» Needs review

I'm working on this.

lauriii’s picture

Status:Needs review» Needs work

No, this doesn't need review :P

lauriii’s picture

Status:Needs work» Needs review
StatusFileSize
new26.82 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
new4.13 KB

Let's see how many test fails we get with this

Status:Needs review» Needs work

The last submitted patch, 173: convert_page_elements-507488-171.patch, failed testing.

lauriii’s picture

Status:Needs work» Needs review
StatusFileSize
new26.65 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
new851 bytes

This should fix at least one kind of fatals. Lets see if there is more to fix

Status:Needs review» Needs work

The last submitted patch, 175: convert_page_elements-507488-175.patch, failed testing.

lauriii’s picture

Status:Needs work» Needs review
StatusFileSize
new29.03 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
new1.78 KB

More fixes for fatal errors

Wim Leers’s picture

Thank you so much for taking this one on, @lauriii!

Status:Needs review» Needs work

The last submitted patch, 177: convert_page_elements-507488-177.patch, failed testing.

lauriii’s picture

Status:Needs work» Needs review
StatusFileSize
new28.62 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,821 pass(es), 279 fail(s), and 73 exception(s).
[ View ]
new1.02 KB

Status:Needs review» Needs work

The last submitted patch, 180: convert_page_elements-507488-180.patch, failed testing.

lauriii’s picture

Status:Needs work» Needs review
StatusFileSize
new39.5 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,901 pass(es), 192 fail(s), and 7 exception(s).
[ View ]
new9.88 KB

The local tasks seem to be quite broken right now. There is $level parameter on the menu_local_tasks() and inside the function the $level variable is being set inside foreach to something else before data is being read from the cache (WTF!). Might be even worth fixing in another issue.

Anyway most of the test failures are because of missing tabs / actions.

pwolanin’s picture

This looks like a new feature - please do the beta evaluation.

dawehner’s picture

In general this looks great.

This issue has been part of an approved larger meta since always, let's not demotivate people

If we talk about the cacheablity, this could be also seen as bug.

  1. +++ b/core/includes/theme.inc
    @@ -1384,14 +1384,6 @@ function template_preprocess_page(&$variables) {
    -  if (!defined('MAINTENANCE_MODE')) {
    -    $variables['action_links']   = menu_get_local_actions();
    -    $variables['tabs']           = menu_local_tabs();
    -  }
    -  else {
    -    $variables['action_links']   = array();
    -    $variables['tabs']           = array();
    -  }

    +++ b/core/themes/seven/templates/page.html.twig
    @@ -63,19 +63,16 @@
    +      {% if page.tabs %}
    +        <nav class="tabs" role="navigation" aria-label="{{ 'Tabs'|t }}">
    +          {{ page.tabs }}
    +        </nav>

    I'm confused, how does that even work? I thought we no longer add them?

  2. +++ b/core/modules/system/src/Plugin/Block/SystemPageActionsBlock.php
    @@ -0,0 +1,141 @@
    +    $this->configFactory = $config_factory;
    ...
    +      $container->get('config.factory'),

    +++ b/core/modules/system/src/Plugin/Block/SystemPageTabsBlock.php
    @@ -0,0 +1,110 @@
    +    $this->configFactory = $config_factory;
    ...
    +      $plugin_definition,
    +      $container->get('config.factory')

    In none of those two examples I understand why we need the config factory

  3. +++ b/core/modules/system/src/Plugin/Block/SystemPageActionsBlock.php
    @@ -0,0 +1,141 @@
    +    $request = $this->requestStack->getCurrentRequest();
    +    $route_name = $request->attributes->get(RouteObjectInterface::ROUTE_NAME);

    The proper way to do it is now to use the route match, which has a getRouteName() method. This probably was a result of earlier versions of the patch

  4. +++ b/core/modules/system/templates/block--system-page-actions-block.html.twig
    +++ b/core/modules/system/templates/block--system-page-actions-block.html.twig
    @@ -0,0 +1,18 @@

    @@ -0,0 +1,18 @@
    +{% extends "@block/block.html.twig" %}
    +{#
    ...
    +{% block content %}

    3> 3> 3> !Twig love! <3 <3 <3 <3

  5. +++ b/core/modules/taxonomy/src/Tests/VocabularyUiTest.php
    @@ -24,7 +24,7 @@ class VocabularyUiTest extends TaxonomyTestBase {
    -
    + ¶

    unnecessary :)

Status:Needs review» Needs work

The last submitted patch, 182: convert_page_elements-507488-182.patch, failed testing.

lauriii’s picture

#184.1: It works because page.tabs is the region ;)

davidhernandez’s picture

This should have a change record for the conversion of any elements into blocks, and cover any significant changes to the page template.

pwolanin’s picture

Can we have details about the cachability bug in the issue summary?

Also - taking a quick look at the patch, it would seem you should include the language as a cache context for the block if that has the rendered tab that may be localized?

lauriii’s picture

Issue summary:View changes
Status:Needs work» Needs review
Issue tags:-Needs change record+Needs beta evaluation
StatusFileSize
new39.97 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,506 pass(es), 198 fail(s), and 37 exception(s).
[ View ]
new11.15 KB

So there was a probelm with Seven because it requires separated blocks for primary and secondary tabs. I made the block configurable so that the user can select which tabs it wants to print. I think I also addressed #184.2,3,5

lauriii’s picture

Setting tags back

Status:Needs review» Needs work

The last submitted patch, 189: convert_page_elements-507488-187.patch, failed testing.

lauriii’s picture

Status:Needs work» Needs review
StatusFileSize
new40.68 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 93,871 pass(es), 194 fail(s), and 28 exception(s).
[ View ]
new613 bytes

Thanks @Xano for helping me on sorting this out. There is follow-up created for the lacking documentation: #2510912: Document how to expose block plugin schemas on BlockPluginInterface

Wim Leers’s picture

Also - taking a quick look at the patch, it would seem you should include the language as a cache context for the block if that has the rendered tab that may be localized?

Not necessary, see #2453059: Set default render cache contexts: 'theme' + 'languages:' . LanguageInterface::TYPE_INTERFACE.

lauriii’s picture

Issue summary:View changes
lauriii’s picture

Issue summary:View changes

Added beta evaluation that I accidentally removed

Status:Needs review» Needs work

The last submitted patch, 192: convert_page_elements-507488-192.patch, failed testing.

tuutti’s picture

Status:Needs work» Needs review
StatusFileSize
new57.96 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,038 pass(es), 31 fail(s), and 21 exception(s).
[ View ]
new17.74 KB

Fixed some failing tests

Status:Needs review» Needs work

The last submitted patch, 197: convert_page_elements-507488-195.patch, failed testing.

lauriii’s picture

Status:Needs work» Needs review
StatusFileSize
new60.41 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,098 pass(es), 26 fail(s), and 16 exception(s).
[ View ]
new6.85 KB

This should go down to only few failing tests now

Status:Needs review» Needs work

The last submitted patch, 199: convert_page_elements-507488-199.patch, failed testing.

lauriii’s picture

Assigned:lauriii» Unassigned
Status:Needs work» Needs review
StatusFileSize
new60.42 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,115 pass(es), 24 fail(s), and 0 exception(s).
[ View ]
new1.08 KB

Status:Needs review» Needs work

The last submitted patch, 201: convert_page_elements-507488-201.patch, failed testing.

Fabianx’s picture

Issue tags:+Performance

Also helps with performance as we can potentially make those blocks cacheable.

lauriii’s picture

Status:Needs work» Needs review
StatusFileSize
new63.26 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,249 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new3.92 KB

We should be gree-ee-ee-n!

Wim Leers’s picture

Ohhhhh! Almost there :)

EDIT: cross-posted with #204: OMGGGGGG!

Status:Needs review» Needs work

The last submitted patch, 204: convert_page_elements-507488-203.patch, failed testing.

lauriii’s picture

Status:Needs work» Needs review
StatusFileSize
new63.26 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,254 pass(es).
[ View ]
new468 bytes

$times_lauriii_has_had_green_patch_when_he_has_said_so = NULL;

Wim Leers’s picture

Status:Needs review» Needs work
  1. +++ b/core/includes/theme.inc
    @@ -1384,14 +1384,6 @@ function template_preprocess_page(&$variables) {
    -  if (!defined('MAINTENANCE_MODE')) {
    -    $variables['action_links']   = menu_get_local_actions();
    -    $variables['tabs']           = menu_local_tabs();
    -  }
    -  else {
    -    $variables['action_links']   = array();
    -    $variables['tabs']           = array();
    -  }

    YAY!

  2. +++ b/core/lib/Drupal/Core/Menu/LocalActionManager.php
    @@ -181,10 +181,11 @@ public function getActionsForRoute($route_appears) {
    -        '#access' => $this->accessManager->checkNamedRoute($route_name, $route_parameters, $this->account),
    +        '#access' => $this->accessManager->checkNamedRoute($route_name, $route_parameters, $this->account, TRUE),

    This does not actually work until #2487600: #access should support AccessResultInterface objects or better has to always use it lands.

    For now, you'd have to do a bit more work: first assign it to a variable, then set #access based on that, and finally, merge the access result object's cacheability metadata with that of the render array.

    Basically:

    $this->renderer->addCacheableDependency($links, $the_access_result_variable);
  3. +++ b/core/modules/system/src/Plugin/Block/SystemPageActionsBlock.php
    @@ -0,0 +1,122 @@
    + *   admin_label = @Translation("Page actions")
    ...
    +class SystemPageActionsBlock extends BlockBase implements ContainerFactoryPluginInterface {
    ...
    +  protected $localActionManager;

    Wouldn't "Local actions" or even just "Actions" make more sense? It's not called "Page actions" anywhere, except here.

  4. +++ b/core/modules/system/src/Plugin/Block/SystemPageActionsBlock.php
    @@ -0,0 +1,122 @@
    +    // The "Page actions" block is never cacheable, due to access checking.
    +    $form['cache']['#disabled'] = TRUE;
    +    $form['cache']['#description'] = $this->t('This block is never cacheable because access checking is needed, it is not configurable.');
    +    $form['cache']['max_age']['#value'] = 0;
    +
    +    return $form;
    +  }
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function getCacheMaxAge() {
    +    // @todo Make me cacheable now that https://drupal.org/node/2287071 has landed.
    +    return 0;
    +  }

    These parts are actually wrong now.

    We still want max-age = 0, but for a different reason: the dynamic local actions/tasks hooks unfortunately do not specify cacheability metadata. That's the current reason.
    That can be fixed in a follow-up (we should open that issue and link to it), but is out of scope to fix here.

    Finally, this missed the route.name cache context.

  5. +++ b/core/modules/system/src/Plugin/Block/SystemPageActionsBlock.php
    @@ -0,0 +1,122 @@
    +
    +
    +
    +}

    Nit: Too many newlines :)

  6. +++ b/core/modules/system/src/Plugin/Block/SystemPageTabsBlock.php
    @@ -0,0 +1,142 @@
    + *   admin_label = @Translation("Page tabs"),
    ...
    +class SystemPageTabsBlock extends BlockBase implements ContainerFactoryPluginInterface {

    Similar remark as above.

  7. +++ b/core/modules/system/src/Plugin/Block/SystemPageTabsBlock.php
    @@ -0,0 +1,142 @@
    +    $form['cache']['#disabled'] = TRUE;
    +    $form['cache']['#description'] = $this->t('This block is never cacheable because access checking is needed, it is not configurable.');
    +    $form['cache']['max_age']['#value'] = 0;
    +
    +    return $form;
    +  }
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function getCacheMaxAge() {
    +    // @todo Make cacheable now that https://drupal.org/node/2287071 has landed.
    +    return 0;
    +  }

    Similar remarks as above.

  8. +++ b/core/modules/tracker/src/Tests/TrackerTest.php
    @@ -150,7 +160,14 @@ function testTrackerUser() {
    +        'config:block.block.1page_tabs_block',

    That "1" looks like a typo :)

lauriii’s picture

Issue summary:View changes
Status:Needs work» Needs review
Issue tags:+Novice
StatusFileSize
new70.31 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch convert_page_elements-507488-209.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new49.51 KB

Added novice task for the change record.

#208.2, 4-8 done

#208.3 I changed them to be called "Local actions" and "Tabs". "Local tabs" doesn't make sense and "Local tasks" is not good for the end users. I also want to keep sanity between the machine name and name used on UI.

Status:Needs review» Needs work

The last submitted patch, 209: convert_page_elements-507488-209.patch, failed testing.

lauriii’s picture

Status:Needs work» Needs review
StatusFileSize
new70.22 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

Just a simple reroll because I had a little old repo

Status:Needs review» Needs work

The last submitted patch, 211: convert_page_elements-507488-211.patch, failed testing.

lokapujya’s picture

+++ b/core/modules/taxonomy/src/Tests/TermTest.php
@@ -311,7 +325,7 @@ function testTermInterface() {
     // Test edit link as accessed from Taxonomy administration pages.
     // Because Simpletest creates its own database when running tests, we know
     // the first edit link found on the listing page is to our term.
-    $this->clickLink(t('Edit'), 1);
+    $this->clickLink(t('Edit'));

Minor: The comment is now wrong; It's not the first edit link.

The order changed because the tab block is placed before local actions. It might be better if the test didn't rely on the order and if we could more directly click the edit tab.

lauriii’s picture

Status:Needs work» Needs review
StatusFileSize
new75.02 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/tests/Drupal/Tests/Core/Menu/LocalActionDefaultTest.php.
[ View ]
new6.23 KB

Status:Needs review» Needs work

The last submitted patch, 214: convert_page_elements-507488-214.patch, failed testing.

lauriii’s picture

Status:Needs work» Needs review
StatusFileSize
new74.47 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,280 pass(es), 5 fail(s), and 0 exception(s).
[ View ]
new406 bytes

Trying to get used to phpstorm without separated visual and insert mode seems to be hard for me :D

Status:Needs review» Needs work

The last submitted patch, 216: convert_page_elements-507488-216.patch, failed testing.

lauriii’s picture

Status:Needs work» Needs review
StatusFileSize
new75.6 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,306 pass(es).
[ View ]
new1015 bytes

We should be green again

lauriii’s picture

Issue tags:-Needs tests
StatusFileSize
new78.38 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,320 pass(es).
[ View ]
new7.72 KB
lauriii’s picture

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

Created change record

lauriii’s picture

lauriii’s picture

Issue summary:View changes
lauriii’s picture

StatusFileSize
new78.2 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,339 pass(es).
[ View ]
new3.14 KB

Fixed some nits on the documentation

Fabianx’s picture

Overall looks great! :)

Some notes (might be x-post):

  1. +++ b/core/themes/seven/templates/page.html.twig
    @@ -50,6 +46,10 @@
    + * - tabs: Tabs linking to any sub-pages beneath the current page (e.g., the
    + *   view and edit tabs when displaying a node).
    + * - action_links: Actions local to the page, such as "Add menu" on the menu
    + *   administration interface.

    Docs are wrong ...

    page.tabs

    and

    page.actions

  2. +++ b/core/themes/seven/templates/page.html.twig
    @@ -63,19 +63,22 @@
    +    {% if page.secondary_tabs %}
    +      <nav class="tabs" role="navigation" aria-label="{{ 'Tabs'|t }}">
    +        {{ page.secondary_tabs }}
    +      </nav>
         {% endif %}

    It feels a little inconsistent to once have this in the block and once not.

    The reason is classy I assume?

lauriii’s picture

Thanks for the review Fabian!

#224.1 Patch on #223 addresses that :)

#224.2 Its region and blocks but we need to add two of them because Seven wants to show primary and secondary tabs in different locations. Maybe #189 interdiff explains this.

Fabianx’s picture

#225: That is right, but why not add the same block template and extend the existing single-block-case one via twig extends?

Fabianx’s picture

Status:Needs review» Reviewed & tested by the community

Besides that question that can be follow-up as markup is not frozen => RTBC

davidhernandez’s picture

Status:Reviewed & tested by the community» Needs work
+++ b/core/modules/system/templates/block--system-page-actions-block.html.twig
@@ -0,0 +1,18 @@
+    <nav class="action-links">{{ action_links }}</nav>

Shouldn't Classy version of all these templates be added, especially since a CSS class is being added in the core version which should be in the Classy version instead.

Xano’s picture

This really quick and short review is powered by Dutch public transport.

  1. +++ b/core/lib/Drupal/Core/Menu/LocalActionManager.php
    @@ -121,8 +129,10 @@ class LocalActionManager extends DefaultPluginManager implements LocalActionMana
    +   *   The renderer service.

    The service bit is misleading. Dependencies can, but are not always also registered as services.

  2. +++ b/core/lib/Drupal/Core/Menu/LocalTaskManager.php
    @@ -123,8 +131,10 @@ class LocalTaskManager extends DefaultPluginManager implements LocalTaskManagerI
    +   *   The renderer service.

    Same here.

  3. +++ b/core/modules/system/config/schema/system.schema.yml
    @@ -353,6 +353,17 @@ block.settings.system_menu_block:*:
    +  label: 'Tabs block'

    Must we say this is a block, or can that be derived from the context?

  4. +++ b/core/modules/system/config/schema/system.schema.yml
    @@ -353,6 +353,17 @@ block.settings.system_menu_block:*:
    +      label: 'Whether primary tabs is shown'

    Whether primary tabs are shown.

  5. +++ b/core/modules/system/config/schema/system.schema.yml
    @@ -353,6 +353,17 @@ block.settings.system_menu_block:*:
    +      label: 'Whether secondary tabs is shown'

    Whether secondary tabs are shown.

  6. +++ b/core/modules/system/src/Plugin/Block/SystemLocalActionsBlock.php
    @@ -0,0 +1,128 @@
    + * Provides a block to display the page actions.

    Call them local actions here as well?

  7. +++ b/core/modules/system/src/Plugin/Block/SystemLocalActionsBlock.php
    @@ -0,0 +1,128 @@
    +    // The "Page actions" block is never cacheable because of hooks creating

    Same here.

    because hooks (no of)

  8. +++ b/core/modules/system/src/Plugin/Block/SystemLocalActionsBlock.php
    @@ -0,0 +1,128 @@
    +    // local actions doesn't provide cacheability metadata.

    don't (no doesn't)

  9. +++ b/core/modules/system/src/Plugin/Block/SystemLocalActionsBlock.php
    @@ -0,0 +1,128 @@
    +    $form['cache']['#description'] = $this->t('This block is never cacheable because of missing cacheability metadata.');

    This seems somewhat technical to mention in the UI. Would it suffice to just say the block isn't cacheable?

  10. +++ b/core/modules/system/src/Plugin/Block/SystemLocalActionsBlock.php
    @@ -0,0 +1,128 @@
    +    // @todo Remove after local action hooks provide cacheability metadata.

    Add this to-do to the disabled caching form element as well.

  11. +++ b/core/modules/system/src/Plugin/Block/SystemTabsBlock.php
    @@ -0,0 +1,145 @@
    + * Provides a block to display the page tabs.

    local tasks

  12. +++ b/core/modules/system/src/Plugin/Block/SystemTabsBlock.php
    @@ -0,0 +1,145 @@
    + *   admin_label = @Translation("Tabs"),

    Local tasks here as well? Maybe we call them tabs in the UI these days (although that casts judgment on how these tasks look)

  13. +++ b/core/modules/system/src/Plugin/Block/SystemTabsBlock.php
    @@ -0,0 +1,145 @@
    +    // The "Page actions" block is never cacheable because of hooks creating

    Local tasks or Tabs (depending on which one we go with).

  14. +++ b/core/modules/system/src/Plugin/Block/SystemTabsBlock.php
    @@ -0,0 +1,145 @@
    +    // @todo Remove after local task hooks provide cacheability metadata.

    Also add this to-do to the disabled caching form element.

  15. +++ b/core/modules/system/src/Tests/Menu/LocalTasksTest.php
    @@ -17,7 +17,28 @@
    +   * The local tasks block used for testing.

    It's not just any object used for testing, it's the class under test. I have started naming properties that contain classes under test sut. Maybe that would improve the readability here as well?

  16. +++ b/core/modules/system/src/Tests/Menu/LocalTasksTest.php
    @@ -48,6 +69,20 @@ protected function assertLocalTasks(array $routes, $level = 0) {
    +   * Asserts that te local tasks on the specified level is not being printed.

    are not

lauriii’s picture

Status:Needs work» Needs review
StatusFileSize
new78.03 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,328 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new9.53 KB

Fixed #224.2 & #228 & #229

#229.3: its there for sanity. All the other block configs are doing it too so it doesn't make sense to break that rule.

#229.13: they've been called as tabs which is confusing everywhere on the theme layer so it makes sense to not change that on this issue

I also tested the patch visually and it seems to not have visual regressions on either Bartik or Seven.

Status:Needs review» Needs work

The last submitted patch, 230: convert_page_elements-507488-230.patch, failed testing.

lauriii’s picture

Status:Needs work» Needs review
StatusFileSize
new77.52 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,326 pass(es).
[ View ]
new650 bytes

Oops, forgot to remove hook_theme hack from the system module

bill richardson’s picture

StatusFileSize
new7.08 KB
new2.82 KB

Looks okay in Seven but seems to have lost styling for tabs in bartik.
See attached images.

bill richardson’s picture

Status:Needs review» Needs work

More investigation with twig debug activated --- we are not using the correct template in seven or bartik -- using the default block.html.twig instead of specific template for theme.

lauriii’s picture

Status:Needs work» Needs review
StatusFileSize
new76.1 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,363 pass(es).
[ View ]
new3.23 KB

This should fix the visual regressions

Wim Leers’s picture

Status:Needs review» Needs work

This is looking great! Only small things :)

  1. +++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
    @@ -269,6 +269,8 @@ public function rebuildThemeData() {
    +        'tabs' => 'Page tabs',
    +        'actions' => 'Page actions',

    Still the old names.

  2. +++ b/core/lib/Drupal/Core/Menu/LocalActionManager.php
    @@ -183,6 +194,7 @@ public function getActionsForRoute($route_appears) {
    +      $access = $this->accessManager->checkNamedRoute($route_name, $route_parameters, $this->account, TRUE);

    @@ -190,10 +202,12 @@ public function getActionsForRoute($route_appears) {
    -        '#access' => $this->accessManager->checkNamedRoute($route_name, $route_parameters, $this->account),
    +        '#access' => $access->isAllowed(),
    ...
    +      $this->renderer->addCacheableDependency($links, $access);

    Let's add a @todo here, pointing out that this crazy dance and dependency on Renderer can be removed once https://www.drupal.org/node/2487600 lands.

  3. +++ b/core/lib/Drupal/Core/Menu/LocalTaskManager.php
    @@ -302,8 +313,8 @@ public function getTasksBuild($current_route_name) {
    +        $access = $this->accessManager->checkNamedRoute($route_name, $route_parameters, $this->account, TRUE);
    +        if ($access->isAllowed()) {
               $active = $this->isRouteActive($current_route_name, $route_name, $route_parameters);

               // The plugin may have been set active in getLocalTasksForRoute() if
    @@ -323,9 +334,11 @@ public function getTasksBuild($current_route_name) {

    @@ -323,9 +334,11 @@ public function getTasksBuild($current_route_name) {
                 '#weight' => $child->getWeight(),
                 '#access' => $access,
               );
    +          $this->renderer->addCacheableDependency($build, $access);

    1) similar @todo as above
    2) the value for #access is not a boolean.

  4. +++ b/core/modules/system/src/Plugin/Block/SystemLocalActionsBlock.php
    @@ -0,0 +1,129 @@
    +   * Creates a SystemPageActionsBlock instance.

    Old name.

  5. +++ b/core/modules/system/src/Plugin/Block/SystemLocalActionsBlock.php
    @@ -0,0 +1,129 @@
    +    // @todo Remove after local action hooks provide cacheability metadata.
    ...
    +    // @todo Remove after local action hooks provide cacheability metadata.

    Let's link to an issue.

  6. +++ b/core/modules/system/src/Plugin/Block/SystemTabsBlock.php
    @@ -0,0 +1,146 @@
    + * Contains \Drupal\system\Plugin\Block\SystemLocalTasksBlock.

    Copy/paste error :)

  7. +++ b/core/modules/system/src/Plugin/Block/SystemTabsBlock.php
    @@ -0,0 +1,146 @@
    +    // The "Page actions" block is never cacheable because of hooks creating

    Old name.

  8. +++ b/core/modules/system/src/Plugin/Block/SystemTabsBlock.php
    @@ -0,0 +1,146 @@
    +    // @todo Remove after local task hooks provide cacheability metadata.
    ...
    +    // @todo Remove after local task hooks provide cacheability metadata.

    Let's link to a follow-up issue.

  9. +++ b/core/modules/system/src/Plugin/Block/SystemTabsBlock.php
    @@ -0,0 +1,146 @@
    +      '#title' => $this->t('Shown tabs'),
    +      '#description' => $this->t('Select local tasks being shown in the block'),

    "Tabs" vs. "Local tasks"

  10. +++ b/core/modules/system/src/Tests/Menu/LocalTasksTest.php
    @@ -135,4 +170,52 @@ public function testPluginLocalTask() {
    +   * Tests that local task blocks are configurable to print a specific level.

    s/print/show/

  11. +++ b/core/modules/system/src/Tests/Menu/LocalTasksTest.php
    @@ -135,4 +170,52 @@ public function testPluginLocalTask() {
    +    // Verify that local tasks in the second doesn't appear.

    s/second/second level/

  12. +++ b/core/themes/classy/templates/block/block--system-local-actions-block.html.twig
    @@ -0,0 +1,18 @@
    + * Default theme implementation for page local actions.

    +++ b/core/themes/classy/templates/block/block--system-tabs-block.html.twig
    @@ -0,0 +1,18 @@
    + * Default theme implementation for page tabs.

    Old names ("page")

  13. +++ b/core/themes/classy/templates/block/block--system-tabs-block.html.twig
    @@ -0,0 +1,18 @@
    + * - tabs: The Tabs linking to any sub-pages beneath the current page (e.g., the

    s/Tabs/tabs/

lauriii’s picture

Status:Needs work» Needs review
StatusFileSize
new76.37 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,371 pass(es).
[ View ]
new6.37 KB

Thanks a lot for the review! Fixed #236

Wim Leers’s picture

Status:Needs review» Needs work
Related issues:+#2511508: Local tasks and actions are now blocks

Manually tested. Works perfectly. Did another round of review, from a theming POV.

Points 1 through 3 are for a follow-up. Could you create a follow-up for that? They will greatly improve the site building experience. Only point 4 still needs to be addressed here.

  1. +++ b/core/themes/bartik/templates/page.html.twig
    @@ -136,15 +134,9 @@
    +            {{ page.tabs }}
                 {{ page.help }}
    -            {% if action_links %}
    -              <ul class="action-links">{{ action_links }}</ul>
    -            {% endif %}
    +            {{ page.actions }}
                 {{ page.content }}

    Note that this means we can combine these four different regions at last! It means no more insanely many regions, instead… just a few regions, and the order of the blocks mattering :)

  2. +++ b/core/themes/classy/templates/layout/page.html.twig
    @@ -110,11 +108,9 @@
    -      {{ tabs }}
    +      {{ page.tabs }}

    -      {% if action_links %}
    -        <nav class="action-links">{{ action_links }}</nav>
    -      {% endif %}
    +      {{ page.actions }}

           {{ page.content }}

    Same here!

  3. +++ b/core/themes/seven/templates/page.html.twig
    @@ -63,19 +62,13 @@
    +    {{ page.secondary_tabs }}
         {{ page.breadcrumb }}

    And here!

  4. +++ b/core/themes/seven/templates/page.html.twig
    @@ -84,9 +77,9 @@
    -      {% if action_links %}
    +      {% if page.actions %}
             <ul class="action-links">
    -          {{ action_links }}
    +          {{ page.actions }}
             </ul>

    This is wrong.

Wim Leers’s picture

+++ b/core/modules/system/src/Plugin/Block/SystemLocalActionsBlock.php
@@ -103,7 +103,7 @@
+    // @todo Remove after https://www.drupal.org/node/2511508 has landed.

@@ -115,7 +115,7 @@
+    // @todo Remove after https://www.drupal.org/node/2511508 has landed.

+++ b/core/modules/system/src/Plugin/Block/SystemTabsBlock.php
@@ -83,7 +83,7 @@
+    // @todo Remove after https://www.drupal.org/node/2511508 has landed.

@@ -95,7 +95,7 @@
+    // @todo Remove after https://www.drupal.org/node/2511508 has landed.

Should be 2511516.

lauriii’s picture

Status:Needs work» Needs review
StatusFileSize
new76.35 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,368 pass(es).
[ View ]
new2.41 KB

Fixed point #238.4 and #239, good catch!

Created follow-up: #2512326: Clean up regions in Classy & Bartik

Wim Leers’s picture

Thanks for creating #2512326: Clean up regions in Classy & Bartik!

No more remarks. Great job :) Back to RTBC. (First RTBC at #227.)

davidhernandez’s picture

Status:Reviewed & tested by the community» Needs work
+++ b/core/themes/classy/templates/block/block--system-local-actions-block.html.twig
@@ -0,0 +1,18 @@
+ * Default theme implementation for local actions.

The Classy templates shouldn't have these comments. They should say "Theme override for..."

+++ b/core/themes/classy/templates/block/block--system-tabs-block.html.twig
@@ -0,0 +1,18 @@
+ * @ingroup themeable

@ingroup should not be there either.

It looks like the templates were moved to Classy, not copied. Are core versions of block--system-local-actions-block.html.twig, block--system-tabs-block.html.twig, etc not needed now?

lauriii’s picture

Status:Needs work» Needs review
StatusFileSize
new77.99 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,381 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new3.02 KB

Fixed :)

Fabianx’s picture

Status:Needs review» Reviewed & tested by the community

Back to RTBC

davidhernandez’s picture

Status:Reviewed & tested by the community» Needs work

Nitpicking now, but...

+++ b/core/modules/system/templates/block--system-local-actions-block.html.twig
@@ -0,0 +1,14 @@
+ * Theme override for local actions.

The system module templates would keep the "Default theme implementation" since they are being registered in system_theme(). Only the Classy ones are changed to say "Theme override" since they are overriding the system module templates.

+++ b/core/themes/classy/templates/block/block--system-tabs-block.html.twig
@@ -4,15 +4,15 @@
+ *   view and edit tabs when displaying a node).*

The * ended up at the end of the line above.

The last submitted patch, 243: convert_page_elements-507488-243.patch, failed testing.

lauriii’s picture

Status:Needs work» Needs review
StatusFileSize
new78.01 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,368 pass(es).
[ View ]
new2.33 KB

Fixed the failing tests and nits from #245

Fabianx’s picture

Status:Needs review» Reviewed & tested by the community

And back to RTBC! Only 51 comments left till #300 ...

davidhernandez’s picture

+++ b/core/modules/system/system.module
@@ -782,6 +790,14 @@ function system_preprocess_block(&$variables) {
+    case 'system_local_actions_block':
+      $variables['action_links'] = $variables['content']['action_links'];
+      break;
+
+    case 'system_tabs_block':
+      $variables['tabs'] = $variables['content']['tabs'];
+      break;
+

Can we feed action_links and tabs into content, instead of creating new variables, since the only thing happening in the templates is replacing content with those variables? Then we wouldn't need to templates, right?

+++ b/core/modules/system/templates/block--system-local-actions-block.html.twig
@@ -0,0 +1,14 @@
+{% block content %}
+  {{ action_links }}
+{% endblock %}
lauriii’s picture

StatusFileSize
new76.31 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,276 pass(es).
[ View ]
new2.98 KB
Fabianx’s picture

Still RTBC, nice work!

davidhernandez’s picture

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs product manager review
StatusFileSize
new18.54 KB

Looking this over again. Overall, everything looks great, but I have a couple nitpicks and one concern.

+++ b/core/themes/classy/templates/block/block--system-local-actions-block.html.twig
@@ -0,0 +1,16 @@
+ * Available variables:
+ * - action_links: Actions local to the page, such as "Add menu" on the menu
+ *   administration interface.

These comments about variables in local actions and system tabs templates are now wrong. The variables do not exist. You have access to the parent template variables, but I don't think we need to copy them all from the block template. That seems like overkill. I would just delete the whole variables section.

+++ b/core/modules/system/templates/page.html.twig
@@ -52,6 +48,8 @@
  * - page.sidebar_second: Items for the second sidebar.
  * - page.footer: Items for the footer region.
  * - page.breadcrumb: Items for the breadcrumb region.
+ * - page.tabs: Items for the tabs region.
+ * - page.actions: Items for the actions region.

The regions listed in the top comment of all the page templates are no longer in order. They should all be in the order they are printed in the template. I think even some that were already there might have been out of order. Lets fix that.

Lastly, I have a concern about the action links being exposed to users in the Block UI. This is what people will see:

Breadcrumbs. Ok. Tabs. Ok. Content. Ok. Actions? Local actions? What is that? Given all the UX testing that happened in Twin Cities, I could definitely see this being a "huh?" for people. Should we rename this or at least make it a little more obvious what someone is affecting if they move/disable this block?

I didn't see if this was discussed already, so if it was, please correct me. I'm tagging for product manager review just to get a sanity check on that.

lauriii’s picture

Issue tags:+Novice

Block UI doesn't support creating additional description texts for blocks which makes it hard to provide any extra info. I'm not sure if tabs should be tabs on the UI but did that because it was called to be tabs previously in the templates. If we rename local actions to something else, how people knowing what local actions are is supposed to be able to find them from the UI if we change it for something else?

It would be nice If anyone else has any suggestions what else local actions because I don't have any. Also tagging the nits to be worked by a new contributor.

Wim Leers’s picture

#252: IMO that goes back to the long-standing problem that our Block placement/layout UI sucks. What we really need here, is live previews (or at least approximate previews). That's the only way this can be solved with elegance, without burdening the user with piles of text. :(

dsnopek’s picture

@Wim Leers: This doesn't help core, but as part of SCOTCH and porting Panopoly to D8, we're planning to implement block previews (like panopoly_magic does in D7) in CTools for D8.

Bojhan’s picture

@laurri That is on purpose, if you need descriptions for this - we are in more trouble. You shouldn't need it.

This will definitely cause usability regressions. What I don't understand is that things that are in the content body (breadcrumbs, local actions, tabs) need to be in separate regions on the blocks page.

lauriii’s picture

@Bojhan: Thats what we're doing right now but we have a follow-up to fix that. It's not in the scope of this issue!

Wim Leers’s picture

@Bojhan: see #238 + #241 about that.

davidhernandez’s picture

@Wim, my concern is not about placing the local actions block. I'm concerned with presenting people with something they may not understand. All of the other blocks they can intuit to some degree or another, and look at their site to understand what they are affecting. Tabs, breadcrumbs, menus, etc. But, how does a site builder figure out what the local actions are, especially since they haven't been presented with this before in Drupal? I'm not even sure if there are any you can see in Bartik out of the box.

Bojhan’s picture

I am quite concerned that we are making this problem, even larger. I know its simply exposing the problem.

However we want to avoid creating this lump of "artificial - not critical, but must fix before RC, issues". That will not get us closer to release. I think we should at the very least have a solid followup plan, if we decide to push this in. Personally I prefer to commit this, without introducing a usability regression - we shouldn't be doing that this late in the release anymore.

Bojhan’s picture

Issue tags:+Usability
lauriii’s picture

@davidhernandez: I know it makes it hard to understand in some cases why they are there. But the situation is the same if they look the template. There's lots of thing being printed directly and people might not know where its coming from. Also removing local actions or local tasks from a custom theme might not have big impact if admin theme is being used. I don't also think that there's many people changing blocks being shown on the admin theme.

lauriii’s picture

Status:Needs work» Needs review
Issue tags:-Novice
StatusFileSize
new76.61 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,717 pass(es).
[ View ]
new5.31 KB

Fixes #252

lauriii’s picture

Profiled this when tabs and actions are being cached:

=== 8.0.x..8.0.x compared (55919cdb0fed9..55919cfa61bb7):

ct  : 92,936|92,936|0|0.0%
wt  : 159,482|160,094|612|0.4%
mu  : 18,570,072|18,570,072|0|0.0%
pmu : 19,736,144|19,736,144|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=55919cdb0fed9&...

=== 8.0.x..cached-tabs compared (55919cdb0fed9..55919d1190958):

ct  : 92,936|88,242|-4,694|-5.1%
wt  : 159,482|151,346|-8,136|-5.1%
mu  : 18,570,072|17,711,328|-858,744|-4.6%
pmu : 19,736,144|18,835,248|-900,896|-4.6%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=55919cdb0fed9&...

effulgentsia’s picture

Re #256/#258: is #2512326: Clean up regions in Classy & Bartik only about removing the regions being added by this issue? In which case, why split it like that instead of making this issue work with the existing regions? Or, is #2512326: Clean up regions in Classy & Bartik about cleaning up regions that are already in HEAD, in which case, why is that a pre-requisite to not adding more temporary regions here?

lauriii’s picture

Its about cleaning up the regions already in HEAD. Problem is if we would do this now, it would be inconsistent between themes and core. But if we want, we could fix this in system module but not in themes.

bill richardson’s picture

I think the question is why create the new regions for seven in this patch --- tabs and actions can just be placed in the already existing content region in seven ---- you just have to make sure that the blocks are placed in the correct order.

lauriii’s picture

In the seven there is no suitable region for primary tabs or secondary tabs. Secondary tabs could be placed on breadcrumb region but does it make any sense? Local actions could be placed on content region.

Wim Leers’s picture

Issue summary:View changes

I put the numbers of #264 in the IS.

@lauriii: which page is that (I suspect front page), and which user (I suspect the root user).

lauriii’s picture

@Wim Leers its block management page which has action links, primary tabs and secondary tabs. Tested on anonymous user. Warm caches but internal page caching turned off.

Wim Leers’s picture

Issue summary:View changes

Great! IS updated.

Note that is an expensive page, and even then, you see a 5% improvement. So it's likely even more (i.e. higher percentage) on cheaper pages.

lauriii’s picture

StatusFileSize
new75.92 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,359 pass(es), 14 fail(s), and 0 exception(s).
[ View ]
new22.91 KB

Local actions -> action links. I also added local actions into content region in seven and bartik and removed tabs and local actions regions from system module and classy and attached blocks to content region.

Status:Needs review» Needs work

The last submitted patch, 272: convert_page_elements-507488-272.patch, failed testing.

lauriii’s picture

Status:Needs work» Needs review
StatusFileSize
new75.92 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,373 pass(es).
[ View ]
new1.33 KB

Just fixed the tests

bill richardson’s picture

A great job but just a few points i noticed during review -
You have still left an actions region in both Bartik and Seven although not used.
You could do without creating a tabs region in Bartik -- could be placed in content region ?
I would prefer a more neutral named region in Seven to hold both primary /secondary tabs, rather than precise named regions to reflect what blocks are placed there ( this was a concern raised by the usability study ) eg
header or page-top,etc.

lauriii’s picture

StatusFileSize
new74.58 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,265 pass(es).
[ View ]
new3.22 KB

Tabs cannot be placed to content because help region is between tabs and actions.

In seven we should convert the regions to be better in the follow-up because it could be done by merging breadcrumb and secondary tabs together.

bill richardson’s picture

Status:Needs review» Reviewed & tested by the community

happy to leave region renaming to a follow up issue - this should now be okay to implement.

Fabianx’s picture

RTBC + 1

Bojhan’s picture

Status:Reviewed & tested by the community» Needs review

#260 is not adressed.

Fabianx’s picture

#279: Well, I don't think new regions are introduced anymore except for when needed, so I don't think this is a huge UX regression anymore?

Do you feel different about that?

Fabianx’s picture

Issue tags:+Needs reroll

Needs reroll if simplytest.me is right

lauriii’s picture

Issue tags:-Needs reroll
StatusFileSize
new74.34 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,259 pass(es).
[ View ]

Just a small reroll.

#279: Can you point out the specific UX regressions that are being made here that haven't been addressed? Or is it just the idea of moving things into being blocks?