diff --git a/src/Plugin/DisplayVariant/BlockDisplayVariant.php b/src/Plugin/DisplayVariant/BlockDisplayVariant.php index 7e5e394..c2be559 100644 --- a/src/Plugin/DisplayVariant/BlockDisplayVariant.php +++ b/src/Plugin/DisplayVariant/BlockDisplayVariant.php @@ -13,6 +13,7 @@ use Drupal\Component\Utility\NestedArray; use Drupal\Component\Utility\SafeMarkup; use Drupal\Component\Uuid\UuidInterface; use Drupal\Core\Cache\Cache; +use Drupal\Core\Cache\CacheableMetadata; use Drupal\Core\Display\VariantBase; use Drupal\Core\Form\FormStateInterface; use Drupal\Core\Plugin\ContainerFactoryPluginInterface; @@ -126,15 +127,13 @@ class BlockDisplayVariant extends VariantBase implements ContextAwareVariantInte * {@inheritdoc} */ public function build() { - $build = []; - - // Default the max page age to permanent. - $max_page_age = Cache::PERMANENT; - $page = $this->executable->getPage(); // Set default page cache keys that include the page and display. - $page_cache_keys = [ + // @todo Make the whole display a pre_render callback to fully benefit from + // render caching and avoid to re-calculate contexts and access for all + // blocks. + $build['regions']['#cache']['keys'] = [ 'page_manager_page', // The page ID. $page->id(), @@ -142,7 +141,9 @@ class BlockDisplayVariant extends VariantBase implements ContextAwareVariantInte // @todo should have an API for this? $this->configuration['uuid'], ]; - $page_cache_contexts = []; + + $cacheability = CacheableMetadata::createFromRenderArray($build['regions']) + ->addCacheableDependency($page); $contexts = $this->getContexts(); foreach ($this->getRegionAssignments() as $region => $blocks) { @@ -160,12 +161,12 @@ class BlockDisplayVariant extends VariantBase implements ContextAwareVariantInte if ($block instanceof ContextAwarePluginInterface) { $this->contextHandler()->applyContextMapping($block, $contexts); } - if (!$block->access($this->account)) { + $access = $block->access($this->account, TRUE); + $cacheability->addCacheableDependency($access); + if (!$access->isAllowed()) { continue; } - $max_age = $block->getCacheMaxAge(); - $block_build = [ '#theme' => 'block', '#attributes' => [], @@ -183,39 +184,25 @@ class BlockDisplayVariant extends VariantBase implements ContextAwareVariantInte // invalidations in case of block configuration changes. 'tags' => Cache::mergeTags($page->getCacheTags(), $block->getCacheTags()), 'contexts' => $block->getCacheContexts(), - 'max-age' => $max_age, + 'max-age' => $block->getCacheMaxAge(), ], ]; - // Build the cache key and a list of all contexts for the whole page. - $page_cache_keys[] = $block_id; - $page_cache_contexts = Cache::mergeContexts($page_cache_contexts, $block_build['#cache']['contexts']); + + // Merge the cacheability metadata of blocks into the page. This helps + // to avoid cache redirects if the blocks have more cache contexts than + // the page, which the page must respect as well. + $cacheability->addCacheableDependency($block); if (!empty($block_build['#configuration']['label'])) { $block_build['#configuration']['label'] = SafeMarkup::checkPlain($block_build['#configuration']['label']); } - - // Update the page max age, set it to the lowest max age of all blocks. - $max_page_age = Cache::mergeMaxAges($max_age, $max_page_age); $build['regions'][$region][$block_id] = $block_build; } } $build['#title'] = $this->renderPageTitle($this->configuration['page_title']); - if ($max_page_age !== 0) { - // If all blocks of this page can be cached, then the max page age is not - // 0. In this case, we additionally cache the whole page, so we need - // to fetch fewer caches. Also explicitly provide the cache contexts, - // additional contexts might still bubble up from the block content, but - // if not, then we save a cache redirection. - // We don't have to set those values in case we can't cache all blocks, - // as they will bubble up from the blocks. - $build['regions']['#cache'] = [ - 'keys' => $page_cache_keys, - 'contexts' => $page_cache_contexts, - 'max-age' => $max_page_age, - ]; - } + $cacheability->applyTo($build['regions']); return $build; } diff --git a/tests/src/Unit/BlockDisplayVariantTest.php b/tests/src/Unit/BlockDisplayVariantTest.php index 20776b5..4811d8e 100644 --- a/tests/src/Unit/BlockDisplayVariantTest.php +++ b/tests/src/Unit/BlockDisplayVariantTest.php @@ -8,6 +8,7 @@ namespace Drupal\Tests\page_manager\Unit; use Drupal\Component\Uuid\UuidInterface; +use Drupal\Core\Access\AccessResult; use Drupal\Core\Block\BlockPluginInterface; use Drupal\Core\Cache\Cache; use Drupal\Core\Cache\Context\CacheContextsManager; @@ -58,11 +59,9 @@ class BlockDisplayVariantTest extends UnitTestCase { } /** - * Tests the build() method. - * * @covers ::build */ - public function testBuildNoCache() { + public function testBuild() { $container = new ContainerBuilder(); $cache_contexts = $this->prophesize(CacheContextsManager::class); $container->set('cache_contexts_manager', $cache_contexts->reveal()); @@ -70,128 +69,9 @@ class BlockDisplayVariantTest extends UnitTestCase { $account = $this->prophesize(AccountInterface::class); + // Define one block that allows access, access varies by permissions. $block1 = $this->prophesize(BlockPluginInterface::class); - $block1->access($account)->willReturn(TRUE); - $block1->build()->willReturn(['#markup' => 'block1_build_value']); - $block1->getConfiguration()->willReturn(['label' => 'Block label']); - $block1->getPluginId()->willReturn('block_plugin_id'); - $block1->getBaseId()->willReturn('block_base_plugin_id'); - $block1->getDerivativeId()->willReturn('block_derivative_plugin_id'); - $block1->getCacheTags()->willReturn(['block_plugin:block_plugin_id']); - $block1->getCacheMaxAge()->willReturn(0); - $block1->getCacheContexts()->willReturn(['url']); - - $block2 = $this->prophesize()->willImplement(ContextAwarePluginInterface::class)->willImplement(BlockPluginInterface::class); - $block2->access($account)->willReturn(FALSE); - $block2->getCacheTags()->willReturn([]); - $block2->getCacheContexts()->shouldNotBeCalled(); - $block2->build()->shouldNotBeCalled(); - - $blocks = [ - 'top' => [ - 'block1' => $block1->reveal(), - 'block2' => $block2->reveal(), - ], - ]; - $block_collection = $this->getMockBuilder(BlockPluginCollection::class) - ->disableOriginalConstructor() - ->getMock(); - $block_collection->expects($this->once()) - ->method('getAllByRegion') - ->willReturn($blocks); - - $context_handler = $this->prophesize(ContextHandlerInterface::class); - $context_handler->applyContextMapping($block2->reveal(), [])->shouldBeCalledTimes(1); - - $uuid_generator = $this->prophesize(UuidInterface::class); - $page_title = 'Page title'; - $token = $this->getMockBuilder(Token::class) - ->disableOriginalConstructor() - ->getMock(); - $display_variant = $this->getMockBuilder(BlockDisplayVariant::class) - ->setConstructorArgs([['page_title' => $page_title], 'test', [], $context_handler->reveal(), $account->reveal(), $uuid_generator->reveal(), $token]) - ->setMethods(['getBlockCollection', 'drupalHtmlClass', 'renderPageTitle']) - ->getMock(); - - $page = $this->prophesize(PageInterface::class); - $page->id()->willReturn('page_id'); - $page->getCacheTags()->willReturn(['page:page_id']); - $page_executable = new PageExecutable($page->reveal()); - $display_variant->setExecutable($page_executable); - - $display_variant->expects($this->once()) - ->method('getBlockCollection') - ->willReturn($block_collection); - $display_variant->expects($this->once()) - ->method('renderPageTitle') - ->with($page_title) - ->willReturn($page_title); - - $expected_build = [ - 'regions' => [ - 'top' => [ - '#prefix' => '
', - '#suffix' => '
', - 'block1' => [ - '#theme' => 'block', - '#attributes' => [], - '#weight' => 0, - '#configuration' => [ - 'label' => 'Block label' - ], - '#plugin_id' => 'block_plugin_id', - '#base_plugin_id' => 'block_base_plugin_id', - '#derivative_plugin_id' => 'block_derivative_plugin_id', - '#cache' => [ - 'keys' => [ - 0 => 'page_manager_page', - 1 => 'page_id', - 2 => 'block', - 3 => 'block1', - ], - 'tags' => [ - 0 => 'block_plugin:block_plugin_id', - 1 => 'page:page_id', - ], - 'contexts' => [ - 0 => 'url', - ], - 'max-age' => 0, - ], - 'content' => [ - '#markup' => 'block1_build_value', - ], - ], - ], - ], - '#title' => 'Page title', - ]; - - // Call build and the #pre_render callback, remove it from the render array - // to simplify the assertion. - $build = $display_variant->build(); - $build['regions']['top']['block1'] = $display_variant->buildBlock($build['regions']['top']['block1']); - unset($build['regions']['top']['block1']['#pre_render']); - - $this->assertSame($expected_build, $build); - } - - - /** - * Tests the build() method when blocks can be cached. - * - * @covers ::build - */ - public function testBuildCache() { - $container = new ContainerBuilder(); - $cache_contexts = $this->prophesize(CacheContextsManager::class); - $container->set('cache_contexts_manager', $cache_contexts->reveal()); - \Drupal::setContainer($container); - - $account = $this->prophesize(AccountInterface::class); - - $block1 = $this->prophesize(BlockPluginInterface::class); - $block1->access($account)->willReturn(TRUE); + $block1->access($account, TRUE)->willReturn(AccessResult::allowed()->cachePerPermissions()); $block1->getConfiguration()->willReturn(['label' => 'Block label']); $block1->getPluginId()->willReturn('block_plugin_id'); $block1->getBaseId()->willReturn('block_base_plugin_id'); @@ -200,15 +80,17 @@ class BlockDisplayVariantTest extends UnitTestCase { $block1->getCacheMaxAge()->willReturn(3600); $block1->getCacheContexts()->willReturn(['url']); + // Define another block that doesn't allow access $block2 = $this->prophesize()->willImplement(ContextAwarePluginInterface::class)->willImplement(BlockPluginInterface::class); - $block2->access($account)->willReturn(TRUE); + $block2->access($account, TRUE)->willReturn(AccessResult::forbidden()->cachePerUser()); $block2->getConfiguration()->willReturn([]); $block2->getPluginId()->willReturn('block_plugin_id'); $block2->getBaseId()->willReturn('block_base_plugin_id'); $block2->getDerivativeId()->willReturn('block_derivative_plugin_id'); - $block2->getCacheContexts()->willReturn([]); - $block2->getCacheMaxAge()->willReturn(Cache::PERMANENT); - $block2->getCacheTags()->willReturn(['block_plugin2:block_plugin_id']); + // The block is not shown, so cacheability metadata is not collected. + $block2->getCacheContexts()->shouldNotBeCalled(); + $block2->getCacheMaxAge()->shouldNotBeCalled(); + $block2->getCacheTags()->shouldNotBeCalled(); $blocks = [ 'top' => [ 'block1' => $block1->reveal(), @@ -232,7 +114,7 @@ class BlockDisplayVariantTest extends UnitTestCase { ->getMock(); $display_variant = $this->getMockBuilder(BlockDisplayVariant::class) ->setConstructorArgs([['page_title' => $page_title, 'uuid' => 'UUID'], 'test', [], $context_handler->reveal(), $account->reveal(), $uuid_generator->reveal(), $token]) - ->setMethods(['getBlockCollection', 'drupalHtmlClass', 'renderPageTitle']) + ->setMethods(['getBlockCollection', 'renderPageTitle']) ->getMock(); $page = $this->prophesize(PageInterface::class); @@ -240,6 +122,12 @@ class BlockDisplayVariantTest extends UnitTestCase { $page->getCacheTags() ->willReturn(['page:page_id']) ->shouldBeCalled(); + $page->getCacheContexts() + ->willReturn([]) + ->shouldBeCalled(); + $page->getCacheMaxAge() + ->willReturn(Cache::PERMANENT) + ->shouldBeCalled(); $page_executable = new PageExecutable($page->reveal()); $display_variant->setExecutable($page_executable); @@ -257,35 +145,32 @@ class BlockDisplayVariantTest extends UnitTestCase { 'contexts' => ['url'], 'max-age' => 3600, ]; - $expected_cache_block2 = [ - 'keys' => ['page_manager_page', 'page_id', 'block', 'block2'], - 'tags' => ['block_plugin2:block_plugin_id', 'page:page_id'], - 'contexts' => [], - 'max-age' => Cache::PERMANENT, - ]; + // The page cacheability metadata contains the access cacheability metadata + // of accessible and non-accessible blocks. Additionally, the cacheability + // metadata is accessible blocks is merged into to avoid cache redirects + // when possible. $expected_cache_page = [ - 'keys' => ['page_manager_page', 'page_id', 'UUID', 'block1', 'block2'], - 'contexts' => ['url'], + 'keys' => ['page_manager_page', 'page_id', 'UUID'], + 'contexts' => ['url', 'user', 'user.permissions'], + 'tags' => ['block_plugin1:block_plugin_id', 'page:page_id'], 'max-age' => 3600, ]; + // Build the variant and ensure that pre_render is set only for the first + // block. $build = $display_variant->build(); $this->assertSame([$display_variant, 'buildBlock'], $build['regions']['top']['block1']['#pre_render'][0]); - $this->assertSame([$display_variant, 'buildBlock'], $build['regions']['top']['block2']['#pre_render'][0]); + $this->assertTrue(empty($build['regions']['top']['block2'])); $this->assertSame($expected_cache_block1, $build['regions']['top']['block1']['#cache']); - $this->assertSame($expected_cache_block2, $build['regions']['top']['block2']['#cache']); $this->assertSame($expected_cache_page, $build['regions']['#cache']); + // Ensure that building the block returns the correct markup. $block1->build()->willReturn([ '#markup' => 'block1_build_value', ]); - $block2->build()->willReturn([]); - $block1_build = $display_variant->buildBlock($build['regions']['top']['block1']); - $block2_build = $display_variant->buildBlock($build['regions']['top']['block2']); $this->assertSame(['#markup' => 'block1_build_value'], $block1_build['content']); - $this->assertSame(['#markup' => '', '#cache' => $expected_cache_block2], $block2_build); } /**