Problem/motivation

Local tasks and actions are hardwired into templates which gives no control to users to move them around.

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 local tasks and actions as blocks that can be moved around and be cached with render caching after sufficient cacheable metadata has been bubbled up from the hooks providing local tasks.

Performance gain (see #264) if local tasks would be cached:

(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!

API changes

Remove hook_menu_local_tasks() since it's unused carry-over from 7.x and redundant to hook_menu_local_tasks_alter() (dynamic) and hook_local_tasks_alter() (discovery)

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
#398 interdiff.txt1.92 KBlauriii
#398 convert_page_elements-507488-398.patch95.04 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,015 pass(es).
[ View ]
#397 increment.txt16.04 KBpwolanin
#397 507488-397.patch94.89 KBpwolanin
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,869 pass(es).
[ View ]
#394 convert_page_elements-507488-394.patch82.53 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,869 pass(es).
[ View ]
#389 interdiff.txt1.15 KBlauriii
#389 convert_page_elements-507488-389.patch82.34 KBlauriii
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch convert_page_elements-507488-389.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#388 interdiff.txt856 byteslauriii
#388 convert_page_elements-507488-388.patch82.32 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,010 pass(es).
[ View ]
#385 interdiff.txt2.85 KBlauriii
#385 convert_page_elements-507488-385.patch82.37 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,000 pass(es).
[ View ]
#382 interdiff.txt425 byteslauriii
#382 convert_page_elements-507488-382.patch82.16 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,011 pass(es).
[ View ]
#380 interdiff.txt1.01 KBlauriii
#380 convert_page_elements-507488-380.patch82.32 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 99,949 pass(es).
[ View ]
#377 interdiff.txt901 byteslauriii
#377 convert_page_elements-507488-377.patch82.13 KBlauriii
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 99,101 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#375 interdiff.txt37.85 KBlauriii
#375 convert_page_elements-507488-375.patch82.12 KBlauriii
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 99,074 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#372 interdiff.txt858 byteslauriii
#372 convert_page_elements-507488-372.patch81.15 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 99,027 pass(es).
[ View ]
#371 interdiff.txt3.85 KBlauriii
#371 convert_page_elements-507488-371.patch81.15 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 99,081 pass(es).
[ View ]
#369 interdiff.txt3.68 KBlauriii
#369 convert_page_elements-507488-369.patch80.07 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,961 pass(es).
[ View ]
#367 interdiff.txt3.01 KBlauriii
#367 convert_page_elements-507488-366.patch80.52 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,960 pass(es).
[ View ]
#364 convert_page_elements-507488-364.patch79.99 KBjibran
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,949 pass(es).
[ 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
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.

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
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
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.

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.

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.

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!

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.

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
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.

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.
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.
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
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

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.

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.

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.

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.

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.

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

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

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!

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
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 :)

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

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

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

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

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
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!

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.

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

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
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

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.

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

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.

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.

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?

davidhernandez’s picture

Looking this over again. So I suggested the change to "Action links" hoping that would be a little more sensical. It's not brilliant, but I think it works a little better. I'm glad we don't have additional regions now for default core and Classy. Tabs and actions are in 'content'. Actions are also now in 'content' in Stark and Bartik, without any apparent visual regression.

So where we stand is the addition of a 'tabs' region in Seven and Bartik. I think we should get sign-off from Seven and Bartik maintainers before committing this. Otherwise, because there is only one region being added now, I'm less concerned about having to clean up regions in a follow up.

Fabianx’s picture

Assigned:Unassigned» webchick
Issue tags:+Needs subsystem maintainer review

Assigning to webchick for 'product manager review'.

Can someone ping Emma and Lewis?

Bojhan’s picture

@laurri Can you chat me up on IRC? Lets review it quickly.

LewisNyman’s picture

Status:Needs review» Needs work

Hey, great work going on here :) Thank for pinging me. I'm glad I had a chance this because we are making a similar mistake that we made on #1869476: Convert global menus (primary links, secondary links) into blocks where we are name the region after the content we expect to be placed in it. This is not a best practice because you can put any content in a region. It was also found to be confusing for users during out formal usability study. We opened an issue to change this here: #2513526: Rename the menu regions.

I don't mind if it's pre_content or something, as long as it's not a describing the content within it.

davidhernandez’s picture

@lewis, it looks like that issues is moving towards header_first _second naming to replace the menu regions. Is that your preference?

Wim Leers’s picture

Status:Needs work» Needs review
StatusFileSize
new74.42 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,263 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
new1.22 KB

Straight reroll. Conflicted with #2512866: CacheContextsManager::optimizeTokens() optimizes ['user', 'user.permissions'] to ['user'] without adding cache tags to invalidate that when the user's roles are modified. Resolved a conflict; the code in that conflict became simpler thanks to #2512866 (the node grants cache context is no longer optimized away as of that issue).

(lauriii is on vacation ATM AFAIK, so let's keep this going.)

Wim Leers’s picture

StatusFileSize
new68.6 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,249 pass(es), 8 fail(s), and 0 exception(s).
[ View ]
new10.41 KB
+++ b/core/lib/Drupal/Core/Menu/LocalActionManager.php
@@ -190,10 +202,13 @@ public function getActionsForRoute($route_appears) {
+      // @todo Remove this after https://www.drupal.org/node/2487600 has landed.
+      $this->renderer->addCacheableDependency($links, $access);

+++ b/core/lib/Drupal/Core/Menu/LocalTaskManager.php
@@ -321,11 +332,15 @@ public function getTasksBuild($current_route_name) {
+          // @todo Remove this after https://www.drupal.org/node/2487600 has
+          //   landed.
+          $this->renderer->addCacheableDependency($build, $access);

#2487600: #access should support AccessResultInterface objects or better has to always use it has also landed in the mean time, so the patch can be simplified further.

That helps shave off some 6 KB from the patch too :)

Wim Leers’s picture

#286:

+++ b/core/themes/bartik/templates/page.html.twig
@@ -136,15 +133,8 @@
             {{ title_suffix }}
-            {% if tabs %}
-              <nav class="tabs" role="navigation" aria-label="{{ 'Tabs'|t }}">
-                {{ tabs }}
-              </nav>
-            {% endif %}
+            {{ page.tabs }}
             {{ page.help }}
-            {% if action_links %}
-              <ul class="action-links">{{ action_links }}</ul>
-            {% endif %}
             {{ page.content }}

That then means we must merge these 3 regions together: tabs, help and content need to be merged together to just content.

lauriii was trying not to do that here because it is technically out of scope.

Do you want us to do that here?

Wim Leers’s picture

Status:Needs work» Needs review
StatusFileSize
new67.88 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch convert_page_elements-507488-293.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new8.31 KB

Fixed the failures.

Wim Leers’s picture

Status:Needs work» Needs review
StatusFileSize
new67.26 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,265 pass(es).
[ View ]

Oops. #293 was diffed against HEAD^, not HEAD.

The #293 interdiff was correct though.

LewisNyman’s picture

@lewis, it looks like that issues is moving towards header_first _second naming to replace the menu regions. Is that your preference?

Yeah, I think that follows the pattern with the other regions.

@Wim If we can merge the regions without causing any visual regressions, then I think this is a good proposal. I don't know if it logically fits within the scope of the issue, I'll leave that call up to you.

Thanks for the hard work!

davidhernandez’s picture

That then means we must merge these 3 regions together

Emphasis added. What exactly prompted this comment? I'm not entirely following along with the code changes you made, Wim. Is there a technical reason now why they have to be merged?

To agree with Lewis, we do not particularly like these regions, but the visual regressions in Bartik is the real issue. We can try to fix that here, but it may delay the issue considerably. I think Lauri said the tabs were the biggest problem, if I remember right, but I haven't tried it.

Wim Leers’s picture

Emphasis added. What exactly prompted this comment?

It was a pure response to Lewis' comment in #286, where he said:

I'm glad I had a chance this because we are making a similar mistake that we made […] where we are naming the region after the content we expect to be placed in it. This is not a best practice because you can put any content in a region. It was also found to be confusing for users during out formal usability study. […]

I don't mind if it's pre_content or something, as long as it's not a describing the content within it.

So, I was merely applying that reasoning to the current patch. The part I quoted in #290 was and still is in the current patch; that's the part of the patch that would have to change if we were to do what Lewis is asking. The patch adds the tabs region, and Lewis would like to not add that new region. So, consequently, that means not adding a specific region: it means either renaming helps to pre_content (like Lewis suggested), or merging help and content together into content (like I suggested). Note that we can do that only now, because only now help becomes adjacent to content!

I'm not entirely following along with the code changes you made, Wim.

The changes in the patch are completely independent from Lewis' review! I haven't addressed Lewis' review yet — all changes I made are just to get the patch green & clean again :)

Is there a technical reason now why they have to be merged?

No, not a technical reason, but merging them follows from what Lewis is asking for AFAICT.

davidhernandez’s picture

@Wim, oh ok. I missed your link to #286. I thought you were saying that because of the changes in the issues you linked to that there was some cache-based reason why we need to merge the regions. I thought that seemed funny.

Wim Leers’s picture

#299: oh, heh, sorry for the confusion.

But, now that we're talking anyway: what are your thoughts on my response to #286, i.e. my proposal in #290 & #299?

davidhernandez’s picture

So may original hangup was with some of the regions being added and the block names. Where we stand now, no new regions are being added to core, which is great, and we have blocks called "Tabs" and "Action Links". I'm ok with "Action Links" until someone can come up with a better name.

That leaves the addition of primary_tabs and secondary_tabs regions in Seven and a tabs region in Bartik. I'm not sure where Lewis stands on those regions for Seven. I think the header_first _second comments were about Bartik.

The changes in this patch are only adding one region, so if we merge tabs and help into content we are getting rid of a region that already exists. I'm just mentioning that for clarify. Looking at Bartik, there is only one bit of CSS for help, but a fair bit for tabs. That will likely take some work to refactor.

If we merge the regions in Bartik, then we don't have to worry about what regions are called what. We just have to deal with Seven.

Merge or no merge, we're left with two questions. Are those regions ok in Seven, and is it ok to add blocks called "Tabs" and "Action Links". I think the second is a webchick or webchick/Bojhan question.

We might want to screenshot what merging those regions in Bartik looks like to see how difficult a change it will be. And when I say difficult I'm thinking less about the physical work and more about how long it could delay this issue.

davidhernandez’s picture

Hmm so I just played with this a bit and moved the "Tabs' block into the content region. I did not notice any visual regression. And looking at the CSS, it looks completely agnostic to its location. Is the only issue that "Help" is normally below tabs so we'd have to move both? What is an easy way to make help text appear in Bartik?

Wim Leers’s picture

What is an easy way to make help text appear in Bartik?

Update a node.

davidhernandez’s picture

Isn't that a status message, not help text?

Wim Leers’s picture

Eh, yes. I don't know what I was thinking. I looked at every single hook_help() implementation… and I don't see any help for a non-admin route.

So, the only way that I know of, to see a Help block in Bartik is to actually make Bartik your admin theme.

Berdir’s picture

You can configure if node add/edit should use the admin theme and you can take the permission away from a user to see the admin theme.

davidhernandez’s picture

StatusFileSize
new82.76 KB
new85.05 KB

This is what I'm seeing before and after the patch and moving Tabs and Help into the Content region.

I'm not seeing any troubling visual regression. The border and padding is lost because the help CSS is specific to the region, not the block. That is an easy fix.

So, yeah, lets merge it. The result should be more shippable, without needing a followup.

Edit: I had the images reversed.

webchick’s picture

Assigned:webchick» Unassigned
Issue tags:+Needs issue summary update

I'm honestly not sure why this is assigned to me. It seems there's plenty of other feedback that still needs addressing. Happy to look once Bojhan, Lewis, et al feel good about this patch, though, or if you think I have something specific to offer, feel free to let me know.

A general note that found the issue summary confusing because it mentions "All the page elements (title, tabs, actions, breadcrumb, messages) are hardwired to core and contrib themes alike" but the patch itself seems to only deal with action links and local tasks? That seems like an odd place to draw the line; the site building benefits would obviously be much greater with something like site name / logo. So maybe adding some more rationale as to why that's being prioritized here.

I will also say that I am generally very nervous about how late in the cycle this big of a change is being proposed, especially given that it's only a small part of the fix to the overall issue of hard-coded theme variables. It leaves me with a lot of questions:

- Do #2476947: Convert "title" page element into blocks and #2005546: Use branding block in place of page template branding variables (site name, slogan, site logo) get us the rest of way there... or are there other required follow-ups?
- What is the likelihood those two get done quickly (in the next couple of weeks) if this one gets in?
- Where will that leave Drupal 8 if they don't get done?
- Can we do those changes in minor releases? (If so, presumably we could do this one as well, so why push this in now?)
- A 5% performance boost is nice, but the block admin page is one that typically only user 1 has access to. Can we show a similarly strong improvement on e.g. a full node page, which would affect a lot more people? That would make it easier to justify the "impact vs. disruption" here.

Also, in scanning through the patch briefly, I didn't see any update hooks, which surprised me. We should check to ensure that existing sites won't lose their local tasks / action links once they do a beta-to-beta upgrade.

dawehner’s picture

- A 5% performance boost is nice, but the block admin page is one that typically only user 1 has access to. Can we show a similarly strong improvement on e.g. a full node page, which would affect a lot more people? That would make it easier to justify the "impact vs. disruption" here.

To be clear, that was not about the block admin page but rather about the actual site.

Gábor Hojtsy’s picture

@webchick: the related issues list is chock full of other issues where the other things are converted, the issue summaries were written at the same time. Eg. site logo, slogan and name was added as a block in #1053648: Convert site elements (site name, slogan, site logo) into blocks. Unfortunately the main site logo/slogan/name display was not replaced with a block, it was just another block added. The replacement was deferred to #2005546: Use branding block in place of page template branding variables (site name, slogan, site logo).

davidhernandez’s picture

@webchick

- I believe those two issues are the last of it. And I think the change in this issue now no longer needs any follow up. We'll make sure the others get done without needing follow ups as well.
- No idea. I've stopped pretending to predict core development progress. But, if I remember correctly the "use branding block" one is almost done. We need to fix some Bartik CSS. The page title one is further from complete, but more attention has been on this one and the "branding" one, so it will get attention when this one is done. (Plus, it's converting only one element instead of a group.)
- I don't think any of the changes leaves us in a broken state. They're all individual changes, so any that don't get done just leaves us without those particular improvements. If we get this one in, but don't get "title" in, we're just left without title converted. It's not all or nothing.
- These changes can't be pushed to a minor release because they require changes to the page template. It is my understanding that all templates are frozen in a release, because changing them would be equivalent to breaking an api.
- See #309

I added the product manager review tag because I wanted to make sure that the blocks and regions we are adding are ok. But, things have actually consolidated quite a bit and avoided adding new default regions. My one open question is about the local actions. That is converted to a block, but I think presenting it to a site builder with that name is a bit problematic. I suggested changing it to "Action Links" as a slight improvement, but I don't know how much clearer it is what that is. What is your opinion on its name?

webchick’s picture

Thanks for the responses.

Oops, right. I forgot we already added the site name / logo / etc. (I searched in the filter for "logo" but that obviously didn't bring up "branding" :)). So it's really only title that would he the outlier then, that's nice.

Regarding minor releases, I think if there was a way to keep the region name the same or something that didn't necessitate changes to page.twig.html, we could do it. However, I see that this is getting legitimate pushback in this issue on this specific case, so attempting to do it pre-release makes sense.

I'm still confused about the performance numbers, and #309 doesn't really clear it up. The issue summary explicitly says "(block management page which has action links, primary tabs and secondary tabs. Tested as anonymous user. Warm caches but internal page caching turned off.)" If that's incorrect, or if there are other numbers somewhere else more public-facing that aren't in the issue summary, could we get that clarified?

I agree "Local actions" seems like a totally confusing Drupalism, so posed a question on Twitter to see what non-Drupal people would call this part of the page: https://twitter.com/webchick/status/621734527493234688

webchick’s picture

...which promptly attracts responses from Drupal people. :P LOL

Wim Leers’s picture

Issue tags:+Needs profiling
StatusFileSize
new67.73 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,461 pass(es).
[ View ]
new3.37 KB

Also, in scanning through the patch briefly, I didn't see any update hooks, which surprised me. We should check to ensure that existing sites won't lose their local tasks / action links once they do a beta-to-beta upgrade.

Because this patch far, far predates the "update path required" policy that went into effect ~2 weeks ago.

Also, because this touches block config entities, just like the critical #2528178: Provide an upgrade path for blocks context IDs #2354889 (context manager), which is supposed to provide a clear example. So, this issue's upgrade path is effectively blocked on that issue.

I'm still confused about the performance numbers, and #309 doesn't really clear it up.

Let's do another bit of profiling to clear that up. I know @lauriii intended to profile it on e.g. the frontpage. Are you still up for that, @lauriii?


So, having checked in with @davidhernandez on IRC, this issue needs four things:

  1. Don't add a new tabs region, and remove the help region, all those blocks can now simply go in the content region. Less regions, less confusing. (Done in this reroll.)
  2. Fix the help CSS (David will do this).
  3. Profiling (hopefully lauriii will do this)
  4. Upgrade path, blocked on #2528178: Provide an upgrade path for blocks context IDs #2354889 (context manager)

P.S.: yay, comment π × 100! Let's not do that again though.

Wim Leers’s picture

Pinged David in IRC, pinged @lauriii via Twitter, and updated #2528178 to notify the people there that that issue now blocks this one: #2528178-41: Provide an upgrade path for blocks context IDs #2354889 (context manager).

mdrummond’s picture

If we're changing the name for the block, I think Action Links makes a lot of sense. More clear than Local Actions. The feedback on the Twitter survey seems to line up with using that as a name.

Would be great to see this get in. I dropped the ball on this and the other block conversions. Very glad to see so many working to make this happen. Would be nice to have Blocks Everywhere for D8. Performance boost and site building sanity, having all the pieces on a page movable as blocks.

webchick’s picture

Filtering out the responses from Drupal people at https://twitter.com/webchick/status/621734527493234688:

- https://twitter.com/SerenitaZhu/status/621806947688316928: Action Link or Main Action I think
- https://twitter.com/bbonfils/status/621773132811079680: Quick actions
- https://twitter.com/helloimbenny/status/621769997354823680: Menu Action
- https://twitter.com/xalking/status/621759048917086209: task/action/content creation, or perhaps even primary action? It seems to be presented that way here...
- https://twitter.com/asberenyi/status/621754583912706048: thinking main action; next step; key step; main thing. Common thread is the main thing most people will need to do next
- https://twitter.com/Shovelita/status/621752375318024192: add element button perhaps
- https://twitter.com/AndrewDuck_/status/621750446055665665: actions I guess from a Rails MVC kinda viewpoint
- https://twitter.com/iamrvazquez/status/621738174130323456: The nav screenshot I call an action. The other screenshot I call a Call To Action. The context is what marks the difference.
- Counter-point: https://twitter.com/PortmanDoe/status/621742889173848064: do you mean non-Drupal AND non-webby? Because as "general public," I'm like "Wtf is call to action?"

So no clear winner. But the word "action" shows up in most of the responses.

Given the guidelines at https://www.drupal.org/node/1089894 recommend only having one of these, maybe call the block "Primary admin action." It's not perfect, since nothing stops them from showing up on a non-admin page, and once in awhile there can be multiple of these (though it's discouraged), but that's generally speaking what they're for.

But as long as the machine name of the block is action_links or whatever, we can debate the user-facing label in another issue without breaking BC, afaik.

davidhernandez’s picture

Probably should be plural. Should we avoid shortening to admin? "Primary administrative actions"? Or Just "Primary actions"? I say if nothing better comes up in the next 3 comments we're going with webchick's suggestion.

lauriii’s picture

Issue tags:-Needs profiling

I'm okay with 'primary admin actions' but just wanted to make clear that local actions might not always be used in administrative purposes.

I did some profiling for this in some other use cases. Again; this issue doesn't create the performance improvement but makes it possible to be done in future. We need hooks providing local tasks/actions to provide cacheable metadata and after its done we can turn on the caching.

The profiling case in this one is user/1 page for anonymous user which has permissions to edit that user. Internal page caching has been turned off, render caching is on.

=== 8.0.x..8.0.x compared (55a8732244c39..55a8733f9d19a):

ct  : 37,603|37,603|0|0.0%
wt  : 75,092|75,017|-75|-0.1%
mu  : 16,769,840|16,769,840|0|0.0%
pmu : 16,875,496|16,875,496|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=55a8732244c39&...

Run 55a8732244c39 uploaded successfully for drupal-perf-lauriii.
Run 55a8735248325 uploaded successfully for drupal-perf-lauriii.

=== 8.0.x..cached-tabs compared (55a8732244c39..55a8735248325):

ct  : 37,603|33,935|-3,668|-9.8%
wt  : 75,092|68,926|-6,166|-8.2%
mu  : 16,769,840|15,742,800|-1,027,040|-6.1%
pmu : 16,875,496|15,824,480|-1,051,016|-6.2%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=55a8732244c39&...

lauriii’s picture

Removing the subsystem maintainer tag since there is already product manager review tag and the comment where this was added says this should be reviewed by the product manger.

Wim Leers’s picture

Thanks @lauriii!

Again; this issue doesn't create the performance improvement but makes it possible to be done in future. We need hooks providing local tasks/actions to provide cacheable metadata and after its done we can turn on the caching.

For that, we have #2511516: Make local tasks and actions hooks provide cacheability metadata, to make the blocks showing them cacheable.

@lauriii: the numbers you posted do show a very nice improvement, so I'm assuming that's with #2511516 already applied then?

Making one of the cheaper pages 10% faster… that is quite impressive at this stage. So this really saves about 4000 function calls. That's quite a lot!

LewisNyman’s picture

Status:Needs review» Needs work
+++ b/core/themes/seven/seven.info.yml
@@ -21,5 +21,7 @@ regions:
   breadcrumb: Breadcrumb
+  primary_tabs: 'Primary tabs'
+  secondary_tabs: 'Secondary tabs'

It looks like Seven's design requires two regions for this content. We can't name them after the tabs though.

The primary tabs appear in the 'content-header' component. So maybe we can call this region 'content-header' or just 'header' to match other themes?

The secondary tabs region appears above the content region, so something like 'pre-content' could make sense.

Fabianx’s picture

I have reviewed the profiling of #321 and it is legit, so this issue would be critical on catch' computer :-D. More seriously: menu_local_actions are very heavy on the system and therefore this is borderline critical.

I see that calls to menu_local_actions completely vanish, so I think the blocks are cached already here.

I think we will need to add max-age=0 to the blocks cacheable metadata in this patch here, to avoid that for now though.

As I fear else we would cache 'too much'.

Wim Leers’s picture

I see that calls to menu_local_actions completely vanish, so I think the blocks are cached already here.

I think we will need to add max-age=0 to the blocks cacheable metadata in this patch here, to avoid that for now though.

But:

+++ b/core/modules/system/src/Plugin/Block/SystemActionLinksBlock.php
@@ -0,0 +1,129 @@
+  public function getCacheMaxAge() {
+    // @todo Remove after https://www.drupal.org/node/2511516 has landed.
+    return 0;
+  }

+++ b/core/modules/system/src/Plugin/Block/SystemTabsBlock.php
@@ -0,0 +1,146 @@
+  public function getCacheMaxAge() {
+    // @todo Remove after https://www.drupal.org/node/2511516 has landed.
+    return 0;
+  }
+

I'm pretty sure @lauriii did the profiling with max-age = 0 uncommented, i.e. I think he pretended that #2511516: Make local tasks and actions hooks provide cacheability metadata, to make the blocks showing them cacheable was already fixed. (I asked him in #321.)

lauriii’s picture

Sorry for not saying this explicit enough :) I turned on the block caching to get the comparison. Thats what Im trying to say on this:

Again; this issue doesn't create the performance improvement but makes it possible to be done in future. We need hooks providing local tasks/actions to provide cacheable metadata and after its done we can turn on the caching.

Wim Leers’s picture

Thanks for confirming, @lauriii!

Fabianx’s picture

#325: Okay that makes a lot of sense.

dawehner’s picture

As said on the d8 critical call we should try to figure out why its actually so slow. I have the feeling/hope that there could be things improved as well.

Wim Leers’s picture

So, from #314:

  1. Don't add a new tabs region, and remove the help region, all those blocks can now simply go in the content region. Less regions, less confusing. (Done in this reroll.)
  2. Fix the help CSS (David will do this).
  3. Profiling (hopefully lauriii will do this)
  4. Upgrade path, blocked on #2528178: Provide an upgrade path for blocks context IDs #2354889 (context manager)

Points 1 and 3 are done. That leaves points 2 and 4.

davidhernandez’s picture

Assigned:Unassigned» davidhernandez

I'll work on 2 and the region and block names if that wasn't done already.

davidhernandez’s picture

Status:Needs work» Needs review
StatusFileSize
new69.52 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch convert_page_elements-507488-331.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new22.51 KB

Ok, made the naming changes. Lauri, I apologize for ever asking you rename things! I moved Bartik's help CSS, and renamed the tabs region in Seven, per Lewis' suggestion.

One thing bothering me is the region inconsistencies I'm seeing. They seem to be all out of order comparing the order they are declared in the info versus printed in page template versus the comment orders. It would be good to fix that. I think it wasn't noticed in Seven because people don't really look at the block placement in Seven.

Also, what are these regions doing in Seven?

  page_top: 'Page top'
  page_bottom: 'Page bottom'
davidhernandez’s picture

Assigned:davidhernandez» Unassigned
Status:Needs work» Needs review
StatusFileSize
new69.32 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,551 pass(es).
[ View ]

Needed to reroll a change to ConfigTranslationOverviewTest.php

davidhernandez’s picture

Another thing I noticed is that the "actions" use a CSS class name "action-links". I debated about changing that, but it is that way in HEAD. Any thoughts on changing it since neither the block machine name nor the block label now have that wording?

joelpittet’s picture

Status:Needs review» Needs work

@davidhernandez +1 to "Action Links" to match the CSS and less name difference baggage/translation.
Edit: that for renaming the block label to that, but I've not thought that through enough but still +1 to that, just don't think it's helpful to this so reverting;)

@Xano made note of this above in #229.15, just highlighting it:

+++ b/core/modules/system/src/Tests/Menu/LocalTasksTest.php
@@ -17,7 +17,28 @@
+  protected $sut;
...
+    $this->sut = $this->drupalPlaceBlock('system_tabs_block', ['id' => 'tabs_block']);

Can we spell this out with a more explicit name. Unless there is a convention I don't know about for whatever a "sut" is?

lauriii’s picture

I guess $sut is not being used in Drupal Core elsewhere yet but its quite commonly used else where and stands for "system under testing" I think. Maybe @Xano can provide more insights if needed.

@davidhernandez: Maybe it could be changed to match the machine name. Not necessary though.

lauriii’s picture

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

Fixing the last "Needs tag"!

lauriii’s picture

lauriii’s picture

Issue summary:View changes
davidhernandez’s picture

Status:Needs work» Needs review

The point remaining from Wim's list is the upgrade function. We can discuss naming convention as we go, but the patch will still need reviewing.

jibran’s picture

I'm confused, what kind of upgrade path are we talking about here? And how is this blocked on context issue? Can someone please elaborate the steps?

jibran’s picture

lauriii’s picture

I guess it should be showing example how to write upgrade paths for block config entities

lauriii’s picture

Issue tags:+Needs tests
StatusFileSize
new75.3 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,647 pass(es).
[ View ]
new5.8 KB

I was working on the upgrade path. We still need test coverage for it.

lauriii’s picture

StatusFileSize
new90.11 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,589 pass(es), 1,128 fail(s), and 0 exception(s).
[ View ]

Just a plain reroll

Pages