Convert page elements (local tasks, actions) into blocks

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

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.

Tagging it up.

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.

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

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

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

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

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.

see also #1947536: Convert drupal_get_breadcrumb() and drupal_set_breadcrumb() to a service

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.

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.

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

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

Is this issue still valid for d8?

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.

NW as per #24.

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?

That sounds like the right place for them.

Eclipse

Update summary.

...moving related issues to their dedicated metadata section.

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

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.

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!

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.

Helps if I post the patch.

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.

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

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)

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.

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.

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.

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.

Bad patch. Hopefully this works.

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

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.

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.

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

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.

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.

Ugh. Here's the actual patch.

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.

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

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.

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

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

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!

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

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

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.

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.

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.

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

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.

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.

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.

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.

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

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.

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

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

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

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.

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

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.

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

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

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.

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

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

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.

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.

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.

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

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.

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

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

Yeah, and then really setting it to needs review.

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?

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?

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?

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

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.

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

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

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

Exactly!

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.

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

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.

Jeff Burnz++

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

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.

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

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

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:

    if (empty($title)) {
      if (is_callable('views_get_page_view')) {
        if ($view = views_get_page_view()) {
          $title = $view->getTitle();
        }
      }
    }

Straight reroll of #94.

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

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

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

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.

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.

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

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

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

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.

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.

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.

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.

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

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

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.

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

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

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

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

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.

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

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

oops

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

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.

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.

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

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

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.

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.