From 0d83597731b44c1b377188681619bc0481f2c867 Mon Sep 17 00:00:00 2001 From: Lee Rowlands Date: Fri, 1 Mar 2019 13:39:47 +1000 Subject: [PATCH] Issue #3030647 by tim.plunkett, phenaproxima, xjm, DyanneNova, bnjmnm, tedbow, dead_arm: Do not add a section when editing an empty layout, or differentiate between new layouts and existing empty layouts --- .../layout_builder/layout_builder.module | 8 ++ .../layout_builder.post_update.php | 7 ++ .../src/Element/LayoutBuilder.php | 19 +--- .../Entity/LayoutBuilderEntityViewDisplay.php | 13 ++- .../src/Form/RevertOverridesForm.php | 7 +- .../src/OverridesSectionStorageInterface.php | 8 ++ .../src/Plugin/Layout/BlankLayout.php | 31 +++++++ .../OverridesSectionStorage.php | 12 ++- .../SectionStorage/SectionStorageBase.php | 8 ++ .../src/SectionListInterface.php | 15 +++ .../SectionStorage/SectionStorageTrait.php | 63 +++++++++++++ .../src/Functional/LayoutBuilderTest.php | 93 ++++++++++++++++++- .../LayoutBuilderTest.php | 6 +- ...outBuilderFieldLayoutCompatibilityTest.php | 2 +- .../src/Kernel/LayoutBuilderInstallTest.php | 2 +- .../Kernel/OverridesSectionStorageTest.php | 32 +++++++ .../tests/src/Kernel/SectionListTraitTest.php | 67 +++++++++++++ .../src/Kernel/SectionStorageTestBase.php | 41 +++++++- 18 files changed, 402 insertions(+), 32 deletions(-) create mode 100644 core/modules/layout_builder/src/Plugin/Layout/BlankLayout.php create mode 100644 core/modules/layout_builder/tests/src/Kernel/SectionListTraitTest.php diff --git a/core/modules/layout_builder/layout_builder.module b/core/modules/layout_builder/layout_builder.module index ba38bf7377f..a7eb3e4e3b1 100644 --- a/core/modules/layout_builder/layout_builder.module +++ b/core/modules/layout_builder/layout_builder.module @@ -293,6 +293,14 @@ function layout_builder_plugin_filter_layout__layout_builder_alter(array &$defin } } +/** + * Implements hook_plugin_filter_TYPE_alter(). + */ +function layout_builder_plugin_filter_layout_alter(array &$definitions, array $extra, $consumer) { + // Hide the blank layout plugin from listings. + unset($definitions['layout_builder_blank']); +} + /** * Implements hook_system_breadcrumb_alter(). */ diff --git a/core/modules/layout_builder/layout_builder.post_update.php b/core/modules/layout_builder/layout_builder.post_update.php index ff4ef098bd7..68b3bc846b7 100644 --- a/core/modules/layout_builder/layout_builder.post_update.php +++ b/core/modules/layout_builder/layout_builder.post_update.php @@ -95,6 +95,13 @@ function layout_builder_post_update_routing_entity_form() { // Empty post-update hook. } +/** + * Clear caches to discover new blank layout plugin. + */ +function layout_builder_post_update_discover_blank_layout_plugin() { + // Empty post-update hook. +} + /** * Clear caches due to routing changes to changing the URLs for defaults. */ diff --git a/core/modules/layout_builder/src/Element/LayoutBuilder.php b/core/modules/layout_builder/src/Element/LayoutBuilder.php index f02f9ebcb24..b92c383526a 100644 --- a/core/modules/layout_builder/src/Element/LayoutBuilder.php +++ b/core/modules/layout_builder/src/Element/LayoutBuilder.php @@ -12,7 +12,6 @@ use Drupal\Core\Url; use Drupal\layout_builder\Context\LayoutBuilderContextTrait; use Drupal\layout_builder\LayoutTempstoreRepositoryInterface; use Drupal\layout_builder\OverridesSectionStorageInterface; -use Drupal\layout_builder\Section; use Drupal\layout_builder\SectionStorageInterface; use Symfony\Component\DependencyInjection\ContainerInterface; @@ -140,20 +139,10 @@ class LayoutBuilder extends RenderElement implements ContainerFactoryPluginInter if ($this->layoutTempstoreRepository->has($section_storage)) { $this->messenger->addWarning($this->t('You have unsaved changes.')); } - // Only add sections if the layout is new and empty. - elseif ($section_storage->count() === 0) { - $sections = []; - // If this is an empty override, copy the sections from the corresponding - // default. - if ($section_storage instanceof OverridesSectionStorageInterface) { - $sections = $section_storage->getDefaultSectionStorage()->getSections(); - } - - // For an empty layout, begin with a single section of one column. - if (!$sections) { - $sections[] = new Section('layout_onecol'); - } - + // If the layout is an override that has not yet been overridden, copy the + // sections from the corresponding default. + elseif ($section_storage instanceof OverridesSectionStorageInterface && !$section_storage->isOverridden()) { + $sections = $section_storage->getDefaultSectionStorage()->getSections(); foreach ($sections as $section) { $section_storage->appendSection($section); } diff --git a/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php index dfc5bf936d6..8843f8fe573 100644 --- a/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php @@ -104,7 +104,14 @@ class LayoutBuilderEntityViewDisplay extends BaseEntityViewDisplay implements La * {@inheritdoc} */ protected function setSections(array $sections) { - $this->setThirdPartySetting('layout_builder', 'sections', array_values($sections)); + // Third-party settings must be completely unset instead of stored as an + // empty array. + if (!$sections) { + $this->unsetThirdPartySetting('layout_builder', 'sections'); + } + else { + $this->setThirdPartySetting('layout_builder', 'sections', array_values($sections)); + } return $this; } @@ -143,9 +150,7 @@ class LayoutBuilderEntityViewDisplay extends BaseEntityViewDisplay implements La } else { // When being disabled, remove all existing section data. - while (count($this) > 0) { - $this->removeSection(0); - } + $this->removeAllSections(); } } } diff --git a/core/modules/layout_builder/src/Form/RevertOverridesForm.php b/core/modules/layout_builder/src/Form/RevertOverridesForm.php index d0e475442d2..7d1b3c9fb9f 100644 --- a/core/modules/layout_builder/src/Form/RevertOverridesForm.php +++ b/core/modules/layout_builder/src/Form/RevertOverridesForm.php @@ -104,10 +104,9 @@ class RevertOverridesForm extends ConfirmFormBase { */ public function submitForm(array &$form, FormStateInterface $form_state) { // Remove all sections. - while ($this->sectionStorage->count()) { - $this->sectionStorage->removeSection(0); - } - $this->sectionStorage->save(); + $this->sectionStorage + ->removeAllSections() + ->save(); $this->layoutTempstoreRepository->delete($this->sectionStorage); $this->messenger->addMessage($this->t('The layout has been reverted back to defaults.')); diff --git a/core/modules/layout_builder/src/OverridesSectionStorageInterface.php b/core/modules/layout_builder/src/OverridesSectionStorageInterface.php index 8d1787998b0..b00d04d488c 100644 --- a/core/modules/layout_builder/src/OverridesSectionStorageInterface.php +++ b/core/modules/layout_builder/src/OverridesSectionStorageInterface.php @@ -23,4 +23,12 @@ interface OverridesSectionStorageInterface extends SectionStorageInterface { */ public function getDefaultSectionStorage(); + /** + * Indicates if overrides are in use. + * + * @return bool + * TRUE if this overrides section storage is in use, otherwise FALSE. + */ + public function isOverridden(); + } diff --git a/core/modules/layout_builder/src/Plugin/Layout/BlankLayout.php b/core/modules/layout_builder/src/Plugin/Layout/BlankLayout.php new file mode 100644 index 00000000000..def5fe12238 --- /dev/null +++ b/core/modules/layout_builder/src/Plugin/Layout/BlankLayout.php @@ -0,0 +1,31 @@ +getDefaultSectionStorage(); $cacheability->addCacheableDependency($default_section_storage)->addCacheableDependency($this); // Check that overrides are enabled and have at least one section. - return $default_section_storage->isOverridable() && count($this); + return $default_section_storage->isOverridable() && $this->isOverridden(); + } + + /** + * {@inheritdoc} + */ + public function isOverridden() { + // If there are any sections at all, including a blank one, this section + // storage has been overridden. Do not use count() as it does not include + // blank sections. + return !empty($this->getSections()); } } diff --git a/core/modules/layout_builder/src/Plugin/SectionStorage/SectionStorageBase.php b/core/modules/layout_builder/src/Plugin/SectionStorage/SectionStorageBase.php index ab50d25f48b..90c9a4250bd 100644 --- a/core/modules/layout_builder/src/Plugin/SectionStorage/SectionStorageBase.php +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/SectionStorageBase.php @@ -102,6 +102,14 @@ abstract class SectionStorageBase extends ContextAwarePluginBase implements Sect return $this; } + /** + * {@inheritdoc} + */ + public function removeAllSections($set_blank = FALSE) { + $this->getSectionList()->removeAllSections($set_blank); + return $this; + } + /** * {@inheritdoc} */ diff --git a/core/modules/layout_builder/src/SectionListInterface.php b/core/modules/layout_builder/src/SectionListInterface.php index 8df586a7aae..767a27ae759 100644 --- a/core/modules/layout_builder/src/SectionListInterface.php +++ b/core/modules/layout_builder/src/SectionListInterface.php @@ -71,4 +71,19 @@ interface SectionListInterface extends \Countable { */ public function removeSection($delta); + /** + * Removes all of the sections. + * + * @param bool $set_blank + * (optional) The default implementation of section lists differentiates + * between a list that has never contained any sections and a list that has + * purposefully had all sections removed in order to remain blank. Passing + * TRUE will mirror ::removeSection() by tracking this as a blank list. + * Passing FALSE will reset the list as though it had never contained any + * sections at all. Defaults to FALSE. + * + * @return $this + */ + public function removeAllSections($set_blank = FALSE); + } diff --git a/core/modules/layout_builder/src/SectionStorage/SectionStorageTrait.php b/core/modules/layout_builder/src/SectionStorage/SectionStorageTrait.php index 36729d2ba62..4cb2b07c308 100644 --- a/core/modules/layout_builder/src/SectionStorage/SectionStorageTrait.php +++ b/core/modules/layout_builder/src/SectionStorage/SectionStorageTrait.php @@ -31,6 +31,10 @@ trait SectionStorageTrait { * {@inheritdoc} */ public function count() { + if ($this->hasBlankSection()) { + return 0; + } + return count($this->getSections()); } @@ -76,6 +80,11 @@ trait SectionStorageTrait { * {@inheritdoc} */ public function insertSection($delta, Section $section) { + // Clear the section list if there is currently a blank section. + if ($this->hasBlankSection()) { + $this->removeAllSections(); + } + if ($this->hasSection($delta)) { // @todo Use https://www.drupal.org/node/66183 once resolved. $start = array_slice($this->getSections(), 0, $delta); @@ -88,13 +97,67 @@ trait SectionStorageTrait { return $this; } + /** + * Adds a blank section to the list. + * + * @return $this + * + * @see \Drupal\layout_builder\Plugin\Layout\BlankLayout + */ + protected function addBlankSection() { + if ($this->hasSection(0)) { + throw new \Exception('A blank section must only be added to an empty list'); + } + + $this->appendSection(new Section('layout_builder_blank')); + return $this; + } + + /** + * Indicates if this section list contains a blank section. + * + * A blank section is used to differentiate the difference between a layout + * that has never been instantiated and one that has purposefully had all + * sections removed. + * + * @return bool + * TRUE if the section list contains a blank section, FALSE otherwise. + * + * @see \Drupal\layout_builder\Plugin\Layout\BlankLayout + */ + protected function hasBlankSection() { + // A blank section will only ever exist when the delta is 0, as added by + // ::removeSection(). + return $this->hasSection(0) && $this->getSection(0)->getLayoutId() === 'layout_builder_blank'; + } + /** * {@inheritdoc} */ public function removeSection($delta) { + // Clear the section list if there is currently a blank section. + if ($this->hasBlankSection()) { + $this->removeAllSections(); + } + $sections = $this->getSections(); unset($sections[$delta]); $this->setSections($sections); + // Add a blank section when the last section is removed. + if (empty($sections)) { + $this->addBlankSection(); + } + return $this; + } + + /** + * {@inheritdoc} + */ + public function removeAllSections($set_blank = FALSE) { + $this->setSections([]); + if ($set_blank) { + $this->addBlankSection(); + } return $this; } diff --git a/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTest.php b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTest.php index 73b8deaa176..53ac02b088e 100644 --- a/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTest.php +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTest.php @@ -869,10 +869,99 @@ class LayoutBuilderTest extends BrowserTestBase { $assert_session->elementsCount('css', '.layout', 1); $assert_session->elementsCount('css', '.layout--twocol', 1); - // The default layout is selected for a new object. + // No layout is selected for a new object. $this->drupalGet('layout-builder-test-simple-config/new'); + $assert_session->elementNotExists('css', '.layout'); + } + + /** + * Tests removing all sections from overrides and defaults. + */ + public function testRemovingAllSections() { + $assert_session = $this->assertSession(); + $page = $this->getSession()->getPage(); + + $this->drupalLogin($this->drupalCreateUser([ + 'configure any layout', + 'administer node display', + ])); + + $field_ui_prefix = 'admin/structure/types/manage/bundle_with_section_field'; + // Enable overrides. + $this->drupalPostForm("$field_ui_prefix/display/default", ['layout[enabled]' => TRUE], 'Save'); + $this->drupalPostForm("$field_ui_prefix/display/default", ['layout[allow_custom]' => TRUE], 'Save'); + + // By default, there is one section. + $this->drupalGet('node/1'); $assert_session->elementsCount('css', '.layout', 1); - $assert_session->elementsCount('css', '.layout--onecol', 1); + $assert_session->pageTextContains('The first node body'); + + $page->clickLink('Layout'); + $assert_session->elementsCount('css', '.layout', 1); + $assert_session->elementsCount('css', '.layout-builder__add-block', 1); + $assert_session->elementsCount('css', '.layout-builder__add-section', 2); + + // Remove the only section from the override. + $page->clickLink('Remove section'); + $page->pressButton('Remove'); + $assert_session->elementsCount('css', '.layout', 0); + $assert_session->elementsCount('css', '.layout-builder__add-block', 0); + $assert_session->elementsCount('css', '.layout-builder__add-section', 1); + + // The override is still used instead of the default, despite being empty. + $page->pressButton('Save layout'); + $assert_session->elementsCount('css', '.layout', 0); + $assert_session->pageTextNotContains('The first node body'); + + $page->clickLink('Layout'); + $assert_session->elementsCount('css', '.layout', 0); + $assert_session->elementsCount('css', '.layout-builder__add-block', 0); + $assert_session->elementsCount('css', '.layout-builder__add-section', 1); + + // Add one section to the override. + $page->clickLink('Add Section'); + $page->clickLink('One column'); + $assert_session->elementsCount('css', '.layout', 1); + $assert_session->elementsCount('css', '.layout-builder__add-block', 1); + $assert_session->elementsCount('css', '.layout-builder__add-section', 2); + + $page->pressButton('Save layout'); + $assert_session->elementsCount('css', '.layout', 1); + $assert_session->pageTextNotContains('The first node body'); + + // By default, the default has one section. + $this->drupalGet("$field_ui_prefix/display/default/layout"); + $assert_session->elementsCount('css', '.layout', 1); + $assert_session->elementsCount('css', '.layout-builder__add-block', 1); + $assert_session->elementsCount('css', '.layout-builder__add-section', 2); + + // Remove the only section from the default. + $page->clickLink('Remove section'); + $page->pressButton('Remove'); + $assert_session->elementsCount('css', '.layout', 0); + $assert_session->elementsCount('css', '.layout-builder__add-block', 0); + $assert_session->elementsCount('css', '.layout-builder__add-section', 1); + + $page->pressButton('Save layout'); + $page->clickLink('Manage layout'); + $assert_session->elementsCount('css', '.layout', 0); + $assert_session->elementsCount('css', '.layout-builder__add-block', 0); + $assert_session->elementsCount('css', '.layout-builder__add-section', 1); + + // The override is still in use. + $this->drupalGet('node/1'); + $assert_session->elementsCount('css', '.layout', 1); + $assert_session->pageTextNotContains('The first node body'); + $page->clickLink('Layout'); + $assert_session->elementsCount('css', '.layout', 1); + $assert_session->elementsCount('css', '.layout-builder__add-block', 1); + $assert_session->elementsCount('css', '.layout-builder__add-section', 2); + + // Revert the override. + $page->clickLink('Revert to defaults'); + $page->pressButton('Revert'); + $assert_session->elementsCount('css', '.layout', 0); + $assert_session->pageTextNotContains('The first node body'); } /** diff --git a/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderTest.php b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderTest.php index 9d2fbddf7b4..9d84c6a7e0d 100644 --- a/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderTest.php +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderTest.php @@ -241,10 +241,10 @@ class LayoutBuilderTest extends WebDriverTestBase { $page->pressButton('Save layout'); - // Removing all sections results in the default layout display being used. + // Removing all sections results in no layout being used. $assert_session->addressEquals($node_url); - $assert_session->elementExists('css', '.layout.layout--onecol'); - $assert_session->pageTextContains('The node body'); + $assert_session->elementNotExists('css', '.layout'); + $assert_session->pageTextNotContains('The node body'); } /** diff --git a/core/modules/layout_builder/tests/src/Kernel/LayoutBuilderFieldLayoutCompatibilityTest.php b/core/modules/layout_builder/tests/src/Kernel/LayoutBuilderFieldLayoutCompatibilityTest.php index 12fd813f4fb..5da3e5d9f49 100644 --- a/core/modules/layout_builder/tests/src/Kernel/LayoutBuilderFieldLayoutCompatibilityTest.php +++ b/core/modules/layout_builder/tests/src/Kernel/LayoutBuilderFieldLayoutCompatibilityTest.php @@ -73,7 +73,7 @@ class LayoutBuilderFieldLayoutCompatibilityTest extends LayoutBuilderCompatibili $this->assertNotEmpty($this->cssSelect('.layout--onecol')); // Removing the layout restores the original rendering of the entity. - $field_list->removeSection(0); + $field_list->removeAllSections(); $this->entity->save(); $this->assertFieldAttributes($this->entity, $expected_fields); } diff --git a/core/modules/layout_builder/tests/src/Kernel/LayoutBuilderInstallTest.php b/core/modules/layout_builder/tests/src/Kernel/LayoutBuilderInstallTest.php index a3ae736da1c..79d36ff9e6f 100644 --- a/core/modules/layout_builder/tests/src/Kernel/LayoutBuilderInstallTest.php +++ b/core/modules/layout_builder/tests/src/Kernel/LayoutBuilderInstallTest.php @@ -51,7 +51,7 @@ class LayoutBuilderInstallTest extends LayoutBuilderCompatibilityTestBase { $this->assertNotEmpty($this->cssSelect('.layout--onecol')); // Removing the layout restores the original rendering of the entity. - $this->entity->get(OverridesSectionStorage::FIELD_NAME)->removeSection(0); + $this->entity->get(OverridesSectionStorage::FIELD_NAME)->removeAllSections(); $this->entity->save(); $this->assertFieldAttributes($this->entity, $expected_fields); diff --git a/core/modules/layout_builder/tests/src/Kernel/OverridesSectionStorageTest.php b/core/modules/layout_builder/tests/src/Kernel/OverridesSectionStorageTest.php index ad466fe7e41..727e8632b2a 100644 --- a/core/modules/layout_builder/tests/src/Kernel/OverridesSectionStorageTest.php +++ b/core/modules/layout_builder/tests/src/Kernel/OverridesSectionStorageTest.php @@ -211,4 +211,36 @@ class OverridesSectionStorageTest extends KernelTestBase { $this->assertSame('default', $result['view_mode']->getContextValue()); } + /** + * @covers ::isOverridden + */ + public function testIsOverridden() { + $display = LayoutBuilderEntityViewDisplay::create([ + 'targetEntityType' => 'entity_test', + 'bundle' => 'entity_test', + 'mode' => 'default', + 'status' => TRUE, + ]); + $display + ->enableLayoutBuilder() + ->setOverridable() + ->save(); + + $entity = EntityTest::create(); + $entity->set(OverridesSectionStorage::FIELD_NAME, [new Section('layout_default')]); + $entity->save(); + $entity = EntityTest::load($entity->id()); + + $context = EntityContext::fromEntity($entity); + $this->plugin->setContext('entity', $context); + + $this->assertTrue($this->plugin->isOverridden()); + $this->plugin->removeSection(0); + $this->assertTrue($this->plugin->isOverridden()); + $this->plugin->removeAllSections(TRUE); + $this->assertTrue($this->plugin->isOverridden()); + $this->plugin->removeAllSections(); + $this->assertFalse($this->plugin->isOverridden()); + } + } diff --git a/core/modules/layout_builder/tests/src/Kernel/SectionListTraitTest.php b/core/modules/layout_builder/tests/src/Kernel/SectionListTraitTest.php new file mode 100644 index 00000000000..bb5b84d4bf7 --- /dev/null +++ b/core/modules/layout_builder/tests/src/Kernel/SectionListTraitTest.php @@ -0,0 +1,67 @@ +setExpectedException(\Exception::class, 'A blank section must only be added to an empty list'); + $this->sectionStorage->addBlankSection(); + } + +} + +class TestSectionList implements SectionListInterface { + + use SectionStorageTrait { + addBlankSection as public; + } + + /** + * An array of sections. + * + * @var \Drupal\layout_builder\Section[] + */ + protected $sections; + + /** + * TestSectionList constructor. + */ + public function __construct(array $sections) { + $this->setSections($sections); + } + + /** + * {@inheritdoc} + */ + protected function setSections(array $sections) { + $this->sections = array_values($sections); + return $sections; + } + + /** + * {@inheritdoc} + */ + public function getSections() { + return $this->sections; + } + +} diff --git a/core/modules/layout_builder/tests/src/Kernel/SectionStorageTestBase.php b/core/modules/layout_builder/tests/src/Kernel/SectionStorageTestBase.php index 6c54d6c9e4f..f943d5183f0 100644 --- a/core/modules/layout_builder/tests/src/Kernel/SectionStorageTestBase.php +++ b/core/modules/layout_builder/tests/src/Kernel/SectionStorageTestBase.php @@ -58,7 +58,7 @@ abstract class SectionStorageTestBase extends EntityKernelTestBase { abstract protected function getSectionStorage(array $section_data); /** - * @covers ::getSections + * Tests ::getSections(). */ public function testGetSections() { $expected = [ @@ -123,6 +123,32 @@ abstract class SectionStorageTestBase extends EntityKernelTestBase { $this->assertSections($expected); } + /** + * @covers ::removeAllSections + * + * @dataProvider providerTestRemoveAllSections + */ + public function testRemoveAllSections($set_blank, $expected) { + if ($set_blank === NULL) { + $this->sectionStorage->removeAllSections(); + } + else { + $this->sectionStorage->removeAllSections($set_blank); + } + $this->assertSections($expected); + } + + /** + * Provides test data for ::testRemoveAllSections(). + */ + public function providerTestRemoveAllSections() { + $data = []; + $data[] = [NULL, []]; + $data[] = [FALSE, []]; + $data[] = [TRUE, [new Section('layout_builder_blank')]]; + return $data; + } + /** * @covers ::removeSection */ @@ -137,6 +163,19 @@ abstract class SectionStorageTestBase extends EntityKernelTestBase { $this->assertSections($expected); } + /** + * @covers ::removeSection + */ + public function testRemoveMultipleSections() { + $expected = [ + new Section('layout_builder_blank'), + ]; + + $this->sectionStorage->removeSection(0); + $this->sectionStorage->removeSection(0); + $this->assertSections($expected); + } + /** * Tests __clone(). */