BlockBase::blockAccess() should return AccessResult instead of a bool

Tagging

Seems like this is no longer postponed?

Just found this from the code comment in BlockBase::access()

Tentatively marking major, because IIRC the flamecharts showed a significant portion of time was spent in checking block access.

Decided to go with keeping blockAccess() . This makes it a) a bit easier to write the method in the first place and b) makes it easier to adapt existing code.

... In general I think we should reavulate to #5 because I can't really believe that access itself is slow ... it always used to be, if I remember correctly, the fact, that access
used to be visibility rules as well, which had a broken context injection.

@wim, those flame charts were because context was gathered for each block individually, that is no longer true. Unless someone has reworked the flame charts to prove this, I'm not sure this is major.

That being said, the logical split between these two functions exists for the sake of making it less likely that a module implementing block plugins would accidentally side-step the entire block access process by making a single function that represents the block's specific access logic. That split still seems relevant to me unless someone has a really good argument to the contrary.

+++ b/core/lib/Drupal/Core/Block/BlockBase.php
@@ -142,15 +133,12 @@ public function access(AccountInterface $account, $return_as_object = FALSE) {
-  protected function blockAccess(AccountInterface $account) {
-    // By default, the block is visible.
-    return TRUE;
-  }
+  abstract protected function blockAccess(AccountInterface $account);

On the one hand I want to agree, on the other hand, we just increased the amount of code every block implementer must write.

Case in point, the rest of this patch... does this really benefit us from a caching perspective?

Eclipse

Access *can* be slow.

Especially at the moment, where access is the only way to prevent a block from being shown (there's an issue for it, in 7.x you could simply return nothing and the block wasn't shown, now you need to implement access for that.. e.g. an empty view).

I'm not entirely sure how access caching would work, but if it would, then we could possibly cache entire page_manager pages without having to load the blocks, their contexts and asking them about access, that would be very interesting.

But yes, we will have to argue why this issue needs to be major to make it in, a normal issue with a change like that is probably not going to work.

Especially at the moment, where access is the only way to prevent a block from being shown (there's an issue for it, in 7.x you could simply return nothing and the block wasn't shown, now you need to implement access for that.. e.g. an empty view).

Well, the problem is that with late rendering, we don't know in beforehand, whether a block is empty or not.

This makes it a) a bit easier to write the method in the first place and b) makes it easier to adapt existing code.

I think that by that you mean the whole $return_as_object thing, right?

Reroll to fix a bunch of nitpicks and a todo.

EDIT: I created an interdiff, but forgot to upload it, and now it's gone. Sorry.

At least major, because this blocks #2429617: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!).

This is a blocker indeed ...

1. +++ b/core/modules/block_content/src/Plugin/Block/BlockContentBlock.php
   @@ -138,6 +138,17 @@ public function blockSubmit($form, FormStateInterface $form_state) {
   +    $uuid = $this->getDerivativeId();
   +    if ($block = $this->entityManager->loadEntityByUuid('block_content', $uuid)) {
   +      return $this->entityManager->getAccessControlHandler('block_content')
   +        ->access($block, 'view');
   

   This means that we need to execute the entity query to get the block id from the uuid to load it twice, there is currently no caching for that, static or otherwise.

2. +++ b/core/modules/help/src/Plugin/Block/HelpBlock.php
   @@ -96,7 +97,12 @@ public static function create(ContainerInterface $container, array $configuratio
      protected function blockAccess(AccountInterface $account) {
        $this->help = $this->getActiveHelp($this->request);
   -    return (bool) $this->help;
   +    if ($this->help) {
   

   Not sure how good that pattern is, but maybe the block content block should do the same and store the loaded entity?

3. +++ b/core/modules/language/src/Plugin/Block/LanguageBlock.php
   @@ -80,7 +81,7 @@ public static function create(ContainerInterface $container, array $configuratio
      protected function blockAccess(AccountInterface $account) {
   -    return $this->languageManager->isMultilingual();
   +    return $this->languageManager->isMultilingual() ? AccessResult::allowed() : AccessResult::forbidden();
      }
   

   We could a the list tag for configurable languages, not sure if that's worth it?

4. +++ b/core/modules/statistics/src/Plugin/Block/StatisticsPopularBlock.php
   @@ -57,22 +58,23 @@ public function defaultConfiguration() {
      protected function blockAccess(AccountInterface $account) {
   +    $access = AccessResult::allowedIfHasPermission($account, 'access content');
        if ($account->hasPermission('access content')) {
          $daytop = $this->configuration['top_day_num'];
          if (!$daytop || !($result = statistics_title_list('daycount', $daytop)) || !($this->day_list = node_title_list($result, $this->t("Today's:")))) {
   -        return FALSE;
   +        return AccessResult::forbidden()->inheritCacheability($access);
   

   Nice trick to get the relevant cache tags, but not exactly obvious that you have to do something like this. Implementing access correctly is quite challenging.

5. +++ b/core/modules/system/src/Plugin/Block/SystemMainBlock.php
   @@ -38,6 +40,13 @@ public function setMainContent(array $main_content) {
       * {@inheritdoc}
       */
   +  protected function blockAccess(AccountInterface $account) {
   +    return AccessResult::allowed();
   +  }
   

   Do we really want to change the default? A lot of blocks have the default implementation copy & pasted.

Rerolled and re-added BlockBase::blockAccess(). Added getEntity() to BlockContentBlock, so the entity is loaded once only. Added the list cache tag to the LanguageBlock access result.

Looking great! This is pretty close :)

1. +++ b/core/lib/Drupal/Core/Block/BlockBase.php
   @@ -121,16 +121,7 @@ public function calculateDependencies() {
   +    $access = $this->blockAccess($account);
   

   Let's add a comment here to document why BlockBase has this method. Something like:

   // We don't want to burden every block to have to handle the $return_as_object parameter. So we do it automatically, so that blocks using this base class only have to worry about returning the AccessResult object.
   

2. +++ b/core/lib/Drupal/Core/Block/BlockBase.php
   @@ -121,16 +121,7 @@ public function calculateDependencies() {
        $access->setCacheMaxAge(0);
   

   We should remove this line now.

3. +++ b/core/modules/block_content/src/Plugin/Block/BlockContentBlock.php
   @@ -132,9 +139,19 @@ public function blockSubmit($form, FormStateInterface $form_state) {
        $uuid = $this->getDerivativeId();
   -    if ($block = $this->entityManager->loadEntityByUuid('block_content', $uuid)) {
   +    if ($block = $this->getEntity()) {
   

   I think the $uuid = … line there is now no longer necessary? :)

4. +++ b/core/modules/block_content/src/Plugin/Block/BlockContentBlock.php
   @@ -147,4 +164,18 @@ public function build() {
   +  }
    }
   

   Nit: We put a newline between the closing brace of the last method and the closing brace of the class.

1. Yes. Added, with a slightly different wording.
2. Oh.. setCacheMaxAge(0) basically says "not cacheable", right? Fixed.
3. Right, fixed.
4. Fixed.

@Berdir also pointed out a few things that should reduce the test fail count, in BlockContentBlock and UserLoginBlock.

Oh, access() returns bool by default. Fixed.

2. Yep :)

One small thing left :) Almost done!

+++ b/core/modules/user/src/Plugin/Block/UserLoginBlock.php
@@ -79,8 +79,7 @@ protected function blockAccess(AccountInterface $account) {
-        ->cachePerRole()
...
+        ->addCacheContexts(['route', 'user.permissions']);

'user.permissions' is not correct. This does not check the permissions, it checks whether the current user has the 'anonymous role. So the right cache context here is actually 'user.roles:anonymous'.

Corrected the cache context.

Excellent!

Added beta evaluation. Hope I got it right.

1. +++ b/core/lib/Drupal/Core/Block/BlockBase.php
   @@ -121,17 +121,10 @@ public function calculateDependencies() {
   +    // Blocks with specific access checking should override blockAccess(). With
   +    // this separation, they do not have to repeat the handling of the
   +    // $return_as_object parameter.
   

   Shouldn't this be in the documentation on the interface?

2. +++ b/core/modules/system/src/Plugin/Block/SystemBrandingBlock.php
   @@ -7,12 +7,12 @@
   +use Drupal\Component\Utility\Xss;
   ...
   -use Drupal\Component\Utility\Xss;
   

   Unrelated change

Moved/rephrased the comment and undid unrelated changes.

Looks straight-forward to me, although it's weird that blocks have this blockAccess() method and other entities do not. However, that's a pre-existing condition, and at least there are docs about it now.

Committed and pushed to 8.0.x. Thanks!

CR added: https://www.drupal.org/node/2472583

This made the frontpage a millisecond slower, but we needed it for correctness.

A millisecond? I'm not sure I see why, we're basically doing the same thing as before? That would mean that merging access results with metadata is so expensive?

Well, I don't know 100% certain this caused it. But I'm seeing way more time being spent in BlockAccessControlHandler, which is initializing a bunch of things it didn't before.

I assumed it was this patch, I didn't do the necessary digging to be 100% certain.

belated: RTBC + 1

If this makes core a little slower that is fine and probably indeed the inheritCacheability calls, but we can still optimize those ...