diff --git a/core/lib/Drupal/Core/Render/Placeholder/PlaceholderStrategyManager.php b/core/lib/Drupal/Core/Render/Placeholder/PlaceholderStrategyManager.php index a553bff..b24ecc2 100644 --- a/core/lib/Drupal/Core/Render/Placeholder/PlaceholderStrategyManager.php +++ b/core/lib/Drupal/Core/Render/Placeholder/PlaceholderStrategyManager.php @@ -19,7 +19,7 @@ class PlaceholderStrategyManager implements PlaceholderStrategyInterface { * * @var \Drupal\Core\Render\Placeholder\PlaceholderStrategyInterface[] */ - protected $placeholderStrategies; + protected $placeholderStrategies = []; /** * Adds a placeholder strategy to use. @@ -39,10 +39,8 @@ public function processPlaceholders(array $placeholders) { return []; } - // In case there are no placeholder strategies, return all placeholders. - if (empty($this->placeholderStrategies)) { - return $placeholders; - } + // Assert that there is at least one strategy. + assert('!empty($this->placeholderStrategies)', 'At least one placeholder strategy needs to be present.'); $new_placeholders = []; @@ -51,6 +49,7 @@ public function processPlaceholders(array $placeholders) { // and this uses a variation of the "chain of responsibility" design pattern. foreach ($this->placeholderStrategies as $strategy) { $processed_placeholders = $strategy->processPlaceholders($placeholders); + assert('array_intersect_key($processed_placeholders, $placeholders) === $processed_placeholders', 'Processed placeholders need to be a subset of all placeholders.'); $placeholders = array_diff_key($placeholders, $processed_placeholders); $new_placeholders += $processed_placeholders; diff --git a/core/lib/Drupal/Core/Render/Renderer.php b/core/lib/Drupal/Core/Render/Renderer.php index 120b204..bb8f1ff 100644 --- a/core/lib/Drupal/Core/Render/Renderer.php +++ b/core/lib/Drupal/Core/Render/Renderer.php @@ -164,8 +164,10 @@ public function renderPlain(&$elements) { * The updated $elements. * * @see ::replacePlaceholders() + * + * @todo Make public as part of https://www.drupal.org/node/2469431 */ - public function renderPlaceholder($placeholder, array $elements) { + protected function renderPlaceholder($placeholder, array $elements) { // Get the render array for the given placeholder $placeholder_elements = $elements['#attached']['placeholders'][$placeholder]; @@ -274,6 +276,12 @@ protected function doRender(&$elements, $is_root_call = FALSE) { $cached_element = $this->renderCache->get($elements); if ($cached_element !== FALSE) { $elements = $cached_element; + // Only when we're in a root (non-recursive) Renderer::render() call, + // placeholders must be processed, to prevent breaking the render cache + // in case of nested elements with #cache set. + if ($is_root_call) { + $this->replacePlaceholders($elements); + } // Mark the element markup as safe if is it a string. if (is_string($elements['#markup'])) { $elements['#markup'] = SafeString::create($elements['#markup']); @@ -284,14 +292,6 @@ protected function doRender(&$elements, $is_root_call = FALSE) { // Render cache hit, so rendering is finished, all necessary info // collected! $context->bubble(); - - // Only when we're in a root (non-recursive) Renderer::render() call, - // placeholders must be processed, to prevent breaking the render cache - // in case of nested elements with #cache set. - if ($is_root_call) { - $this->replacePlaceholders($elements); - } - return $elements['#markup']; } } @@ -533,16 +533,6 @@ protected function doRender(&$elements, $is_root_call = FALSE) { $this->renderCache->set($elements, $pre_bubbling_elements); } - if ($is_root_call) { - // @todo remove as part of https://www.drupal.org/node/2511330. - if ($context->count() !== 1) { - throw new \LogicException('A stray drupal_render() invocation with $is_root_call = TRUE is causing bubbling of attached assets to break.'); - } - } - - // Rendering is finished, all necessary info collected! - $context->bubble(); - // Only when we're in a root (non-recursive) Renderer::render() call, // placeholders must be processed, to prevent breaking the render cache in // case of nested elements with #cache set. @@ -554,8 +544,15 @@ protected function doRender(&$elements, $is_root_call = FALSE) { // that is handled earlier in Renderer::render(). if ($is_root_call) { $this->replacePlaceholders($elements); + // @todo remove as part of https://www.drupal.org/node/2511330. + if ($context->count() !== 1) { + throw new \LogicException('A stray drupal_render() invocation with $is_root_call = TRUE is causing bubbling of attached assets to break.'); + } } + // Rendering is finished, all necessary info collected! + $context->bubble(); + $elements['#printed'] = TRUE; return $elements['#markup']; } diff --git a/core/tests/Drupal/Tests/Core/Render/Placeholder/PlaceholderStrategyManagerTest.php b/core/tests/Drupal/Tests/Core/Render/Placeholder/PlaceholderStrategyManagerTest.php index 4018a0b..9e2d0f7 100644 --- a/core/tests/Drupal/Tests/Core/Render/Placeholder/PlaceholderStrategyManagerTest.php +++ b/core/tests/Drupal/Tests/Core/Render/Placeholder/PlaceholderStrategyManagerTest.php @@ -23,13 +23,13 @@ class PlaceholderStrategyManagerTest extends UnitTestCase { * @dataProvider providerProcessPlaceholders */ public function testProcessPlaceholdersSingleFlush($strategies, $placeholders, $result) { - $render_strategy_manager = new PlaceholderStrategyManager(); + $placeholder_strategy_manager = new PlaceholderStrategyManager(); foreach ($strategies as $strategy) { - $render_strategy_manager->addPlaceholderStrategy($strategy); + $placeholder_strategy_manager->addPlaceholderStrategy($strategy); } - $this->assertEquals($result, $render_strategy_manager->processPlaceholders($placeholders)); + $this->assertEquals($result, $placeholder_strategy_manager->processPlaceholders($placeholders)); } /** @@ -43,12 +43,6 @@ public function providerProcessPlaceholders() { // Empty placeholders. $data[] = [[], [], []]; - // Placeholders but no strategies defined. - $placeholders = [ - 'ignore-me' => ['#markup' => 'I-am-a-llama-that-will-be-ignored-by-the-placeholder-strategy-manager.'], - ]; - $data[] = [[], $placeholders, $placeholders]; - // Placeholder removing strategy. $placeholders = [ 'remove-me' => ['#markup' => 'I-am-a-llama-that-will-be-removed-sad-face.'], @@ -91,6 +85,9 @@ public function providerProcessPlaceholders() { $esi_strategy = $prophecy->reveal(); $prophecy = $this->prophesize('\Drupal\Core\Render\Placeholder\PlaceholderStrategyInterface'); + $prophecy->processPlaceholders($placeholders)->shouldNotBeCalled(); + $prophecy->processPlaceholders($result)->shouldNotBeCalled(); + $prophecy->processPlaceholders([])->shouldNotBeCalled(); $single_flush_strategy = $prophecy->reveal(); $data[] = [[$esi_strategy, $single_flush_strategy], $placeholders, $result]; @@ -126,4 +123,47 @@ public function providerProcessPlaceholders() { return $data; } + /** + * @covers ::processPlaceholders + * + * @expectedException \AssertionError + * @expectedExceptionMessage At least one placeholder strategy needs to be present. + */ + public function testProcessPlaceholdersNoStrategies() { + // Placeholders but no strategies defined. + $placeholders = [ + 'assert-me' => ['#markup' => 'I-am-a-llama-that-will-lead-to-an-assertion-by-the-placeholder-strategy-manager.'], + ]; + + $placeholder_strategy_manager = new PlaceholderStrategyManager(); + $placeholder_strategy_manager->processPlaceholders($placeholders); + } + + /** + * @covers ::processPlaceholders + * + * @expectedException \AssertionError + * @expectedExceptionMessage Processed placeholders need to be a subset of all placeholders. + */ + public function testProcessPlaceholdersWithRoguePlaceholderStrategy() { + // Placeholders but no strategies defined. + $placeholders = [ + 'assert-me' => ['#markup' => 'llama'], + ]; + + $result = [ + 'assert-me' => ['#markup' => 'llama'], + 'new-placeholder' => ['#markup' => 'rogue llama'], + ]; + + $prophecy = $this->prophesize('\Drupal\Core\Render\Placeholder\PlaceholderStrategyInterface'); + $prophecy->processPlaceholders($placeholders)->willReturn($result); + $rogue_strategy = $prophecy->reveal(); + + $placeholder_strategy_manager = new PlaceholderStrategyManager(); + $placeholder_strategy_manager->addPlaceholderStrategy($rogue_strategy); + $placeholder_strategy_manager->processPlaceholders($placeholders); + } + + }