Issue #3208766: Add UUID to sections
Merge request reports
Activity
added 1 commit
- 85381271 - Pulled in the demo_umami config updates from another branch.
added 1 commit
- ba9a47c5 - Fixed the content moderation test. Added a missing comment.
added 1 commit
- 153dc6c1 - Fix section declaration in multiple_sections test
added 15 commits
-
04537cce...a01ef884 - 9 commits from branch
project:11.x
- f5241f43 - Added changes from drupal-3208766/3208766-d10-key-sections-by-uuid.
- 6c079d17 - Pulled in the demo_umami config updates from another branch.
- 814ac22a - Fixed the content moderation test. Added a missing comment.
- 76d507a3 - Fixed another test.
- 39806b5a - Fix section declaration in multiple_sections test
- ee0bf591 - Fix array declaration
Toggle commit list-
04537cce...a01ef884 - 9 commits from branch
added 1 commit
- ffa90f25 - Small refactor of previous commits to take advantage of chaining and weight is...
added 92 commits
-
ffa90f25...58233075 - 85 commits from branch
project:11.x
- 9852aef0 - Added changes from drupal-3208766/3208766-d10-key-sections-by-uuid.
- d6118ba3 - Pulled in the demo_umami config updates from another branch.
- 0b53a20a - Fixed the content moderation test. Added a missing comment.
- 6576b679 - Fixed another test.
- bde98087 - Fix section declaration in multiple_sections test
- 45b0f8e2 - Fix array declaration
- 189bf5ab - Small refactor of previous commits to take advantage of chaining and weight is...
Toggle commit list-
ffa90f25...58233075 - 85 commits from branch
added 497 commits
-
189bf5ab...005c0e91 - 490 commits from branch
project:11.x
- 9f27898a - Added changes from drupal-3208766/3208766-d10-key-sections-by-uuid.
- 155e456d - Pulled in the demo_umami config updates from another branch.
- 4a99e303 - Fixed the content moderation test. Added a missing comment.
- e8ed902c - Fixed another test.
- 75e3dd21 - Fix section declaration in multiple_sections test
- 9ee0d6e4 - Fix array declaration
- 0120ae35 - Small refactor of previous commits to take advantage of chaining and weight is...
Toggle commit list-
189bf5ab...005c0e91 - 490 commits from branch
added 1 commit
- 55326d60 - fix core/profiles/demo_umami/config/install/core.entity_view_display.node.recipe.full.yml
added 11 commits
-
55326d60...93eb2340 - 3 commits from branch
project:11.x
- 6e2cda7e - Added changes from drupal-3208766/3208766-d10-key-sections-by-uuid.
- ad839dec - Pulled in the demo_umami config updates from another branch.
- 236db345 - Fixed the content moderation test. Added a missing comment.
- 81ce5ccc - Fixed another test.
- cc61380b - Fix section declaration in multiple_sections test
- b0db863e - Fix array declaration
- 49522745 - Small refactor of previous commits to take advantage of chaining and weight is...
- 6ef71532 - fix core/profiles/demo_umami/config/install/core.entity_view_display.node.recipe.full.yml
Toggle commit list-
55326d60...93eb2340 - 3 commits from branch
mentioned in merge request !3056 (closed)
mentioned in merge request !3857 (closed)
added 9 commits
- 9f27898a - Added changes from drupal-3208766/3208766-d10-key-sections-by-uuid.
- 155e456d - Pulled in the demo_umami config updates from another branch.
- 4a99e303 - Fixed the content moderation test. Added a missing comment.
- e8ed902c - Fixed another test.
- 75e3dd21 - Fix section declaration in multiple_sections test
- 9ee0d6e4 - Fix array declaration
- 0120ae35 - Small refactor of previous commits to take advantage of chaining and weight is...
- 55326d60 - fix core/profiles/demo_umami/config/install/core.entity_view_display.node.recipe.full.yml
- 74f6a138 - fix uuid for blank section
Toggle commit listadded 32 commits
-
74f6a138...ba666c8d - 23 commits from branch
project:11.x
- 40890753 - Added changes from drupal-3208766/3208766-d10-key-sections-by-uuid.
- 4832544b - Pulled in the demo_umami config updates from another branch.
- 05310259 - Fixed the content moderation test. Added a missing comment.
- 6824be55 - Fixed another test.
- f4beb04b - Fix section declaration in multiple_sections test
- 3a93786d - Fix array declaration
- 32b2778a - Small refactor of previous commits to take advantage of chaining and weight is...
- 8fcd5078 - fix core/profiles/demo_umami/config/install/core.entity_view_display.node.recipe.full.yml
- 857d2a9b - fix uuid for blank section
Toggle commit list-
74f6a138...ba666c8d - 23 commits from branch
added 1 commit
- 61235b38 - use \Drupal::service(uuid)->generate() for generating uuids for section
added 21 commits
-
61235b38...645d5de9 - 11 commits from branch
project:11.x
- 2e0df267 - Added changes from drupal-3208766/3208766-d10-key-sections-by-uuid.
- 6903f680 - Pulled in the demo_umami config updates from another branch.
- c969a9bf - Fixed the content moderation test. Added a missing comment.
- 1dfe2d87 - Fixed another test.
- 4462565b - Fix section declaration in multiple_sections test
- 3f803494 - Fix array declaration
- 9a152527 - Small refactor of previous commits to take advantage of chaining and weight is...
- caed014a - fix core/profiles/demo_umami/config/install/core.entity_view_display.node.recipe.full.yml
- 3351adc4 - fix uuid for blank section
- 4380f5f2 - use \Drupal::service(uuid)->generate() for generating uuids for section
Toggle commit list-
61235b38...645d5de9 - 11 commits from branch
added 1 commit
- c13ef166 - add missing weight of a section in core.entity_view_display.node.recipe.full
added 1 commit
- cffa40ab - fix uuids in test and add a random uuid for blank section
added 1 commit
- 91c1cc4a - use \Drupal::service(uuid)->generate() to generate uuid for blank section and...
113 113 $this->unsetThirdPartySetting('layout_builder', 'sections'); 114 114 } 115 115 else { 116 $this->setThirdPartySetting('layout_builder', 'sections', array_values($sections)); 116 foreach (array_values($sections) as $weight => $section) { 117 $section->setWeight($weight); 118 $sections[$section->getUuid()] = $section; With this change there will be a BC break and we'll have to modify all the logic related to
$delta
used in many methods like\Drupal\layout_builder\SectionListInterface::getSection
,\Drupal\layout_builder\SectionListInterface::insertSection
,\Drupal\layout_builder\SectionListInterface::appendSection
because now the keys are uuids and not the indexes.I think we should go for the 2nd option mentioned in 3rd point of Proposed resolution in Issue Summary i.e. 'create a separate LayoutSectionItemList::getSectionsByUuid() method to get the sections keyed by their uuid'.
Edited by Kunal Sachdevchanged this line in version 23 of the diff
The issue with not storing LB settings in EVDs using sections keyed by UUID is that we lose the ability to compare that config in a meaningful way. If sections are keyed by UUID in that config, you can track changes for something like Config Split (which is my primary use case and reason for working on this). If the sections are keyed by delta (as they currently are) there is no way to reason about what has changed when comparing two versions of the configuration.
But I think for that use case we have a new method that can be used to get sections keyed by uuid, see
LayoutSectionItemList::getSectionsByUuid()
.Edited by Kunal SachdevOk, now I understand that we need to store the sections with uuids as keys and therefore I deleted my commits in which I changed it to store the sections with indexes as keys. But now we'll have to modify all the logic related to
$delta
used in many methods like\Drupal\layout_builder\SectionListInterface::getSection
,\Drupal\layout_builder\SectionListInterface::insertSection
,\Drupal\layout_builder\SectionListInterface::appendSection
because the keys are uuids and not the indexes.Edited by Kunal Sachdev
added 25 commits
-
f252fb27...7cb99a11 - 10 commits from branch
project:11.x
- 7cb99a11...59caac95 - 5 earlier commits
- 0a8222e0 - Fix array declaration
- fda2765f - Small refactor of previous commits to take advantage of chaining and weight is...
- 43a47351 - fix core/profiles/demo_umami/config/install/core.entity_view_display.node.recipe.full.yml
- cba4a7ef - fix uuid for blank section
- d7c379cf - use \Drupal::service(uuid)->generate() for generating uuids for section
- 64dde21f - add missing weight of a section in core.entity_view_display.node.recipe.full
- 979fd797 - fix uuids in test and add a random uuid for blank section
- 1423fe40 - use \Drupal::service(uuid)->generate() to generate uuid for blank section and...
- a871401a - phpcs fix
- 6fcadc37 - use indexes instead of uuids as keys in LayoutSectionItemList::getSections()...
Toggle commit list-
f252fb27...7cb99a11 - 10 commits from branch
added 1 commit
- 16c43a03 - re-key every section in sections array after removing a section| fix some tests
added 1 commit
- e2e66a1f - fix core.entity_view_display.entity_test.bundle_with_extra_fields.default.yml
added 1 commit
- f516f222 - new method SectionListInterface::getSectionsByUuid() and its test
88 $db = \Drupal::database(); 89 $entity_type_manager = \Drupal::entityTypeManager(); 90 91 $section_column = 'layout_builder__layout_section'; 92 93 // Get a list of content entities. 94 $entity_types = array_filter($entity_type_manager 95 ->getDefinitions(), function ($value) { 96 return $value instanceof ContentEntityType; 97 }); 98 99 $tables = []; 100 101 // Compile a list of tables to update. 102 /** @var Drupal\Core\Entity\ContentEntityType $entity_type */ 103 foreach ($entity_types as $key => $entity_type) { - Comment on lines +93 to +103
This is one WILD update hook
🤯Impressive!
I don't see how this could ever land in Drupal core without an update path test precisely because it's so impressive: it touches potentially all content entities' DB tables directly 🦹
Edited by Wim Leers I second this concern. Here's a benchmark of a moderately large site:
www-data@b94f4734f2ca:~/html$ time drush updb ---------------- ----------- --------------- ------------- Module Update ID Type Description ---------------- ----------- --------------- ------------- layout_builder 9001 hook_update_n 9001 - ---------------- ----------- --------------- ------------- Do you wish to run the specified pending updates? (yes/no) [yes]: > > [notice] Update started: layout_builder_update_9001 > [notice] Updated 55028 layout override revisions; 1 kv_expire entries in 820 seconds <-- I patched these counters into the patch. > [notice] Update completed: layout_builder_update_9001 [success] Finished performing updates. real 13m50.438s user 0m39.666s sys 0m4.250s
Edited by Luke Leberchanged this line in version 30 of the diff
115 115 else { 116 116 foreach (array_values($sections) as $weight => $section) { 117 117 $section->setWeight($weight); 118 $sections[$section->getUuid()] = $section; @kunal.sachdev Specifically not setting the UUID as the key here will cause the exported config to not use the UUID as the key. This will make the config comparison use case impossible. If we don't want to store it by UUID in
LayoutSectionItemList
, that is less of an issue for me, but this is critical to supporting our use case.
added 1 commit
- 420b0bdb - fix methods in LayoutBuilderEntityViewDisplay
added 1 commit
- b4c4dabb - more changes in LayoutBuilderEntityViewDisplay
added 1 commit
- 70a5e3da - new method SectionListInterface::getSectionByDelta($delta) and use it instead...
added 1 commit
- d1673b41 - dont use strict checking for delta because sometimes its type is string
46 if (Uuid::isValid($uuid)) { 47 foreach ($this->getSections() as $section) { 48 if ($section->getUuid() === $uuid) { 49 return $section; 50 } 51 } 52 } 53 54 throw new \Exception(sprintf('Invalid uuid "%s"', $uuid)); 43 55 } 44 56 45 return $this->getSections()[$delta]; 57 /** 58 * {@inheritdoc} 59 */ 60 public function getSectionByDelta($delta) { Discussed this new method with @lauriii today, and he had a good point. Instead of adding a new method, callers should use
getSections()[$delta]
instead. This also raises the point that the list of sections should be sorted by weight, so that if someone runsreset($whatever->getSections())
, they get the first one. (This could also be used instead of all the instances ofgetSectionsByDelta(0)
that are in the MR currently)changed this line in version 44 of the diff
40 public function getSection($delta) { 41 if (!$this->hasSection($delta)) { 42 throw new \OutOfBoundsException(sprintf('Invalid delta "%s"', $delta)); 42 public function getSection($uuid) { 43 if (is_int($uuid)) { 44 @trigger_error('Calling ' . __FUNCTION__ . '() with delta as an argument is deprecated in drupal:10.2.0 and is removed from drupal:11.0.0. Instead you should pass uuid or if you want to get section by delta then use \Drupal\layout_builder\SectionListTrait::getSectionByDelta($delta). See https://www.drupal.org/node/3401886', E_USER_DEPRECATED); 45 } 46 if (Uuid::isValid($uuid)) { 47 foreach ($this->getSections() as $section) { 48 if ($section->getUuid() === $uuid) { 49 return $section; 50 } 51 } 52 } 53 54 throw new \Exception(sprintf('Invalid uuid "%s"', $uuid)); changed this line in version 52 of the diff
105 141 throw new \Exception('A blank section must only be added to an empty list'); 106 142 } 107 143 108 $this->appendSection(new Section('layout_builder_blank')); 144 $this->appendSection((new Section('layout_builder_blank'))->setUuid(\Drupal::service('uuid')->generate())->setWeight(0)); There are now a lot more places that have to call
->setUuid(\Drupal::service('uuid')->generate())
than before. However,Section::fromArray()
handles it nicely for you, if you were lazy you could useSection::fromArray(['layout_id' => 'layout_builder_blank'])
instead ofnew
.At some point in the development of LB we introduced a
Section::create()
method but it didn't make it into core. Is this enough a reason to bring that idea back?I think it's a good idea. I also discussed it in scrum and we decided to go with this idea so I am going to work on this.
Edited by Kunal Sachdevchanged this line in version 48 of the diff
added 113 commits
-
d1673b41...502c857b - 87 commits from branch
project:11.x
- 502c857b...729ceed3 - 16 earlier commits
- f2905da8 - phpcs fix
- 0f4e765e - fix methods in LayoutBuilderEntityViewDisplay
- f3e58f0d - more changes in LayoutBuilderEntityViewDisplay
- 376928ce - fix layout_builder_install()
- fac7e9f9 - new method SectionListInterface::getSectionByDelta($delta) and use it instead...
- e110bd34 - test changes work-in-progress
- edefb032 - more test fixes and new test coverage
- bd5a7385 - more improvements and test fixes
- fa3a853b - dont use strict checking for delta because sometimes its type is string
- 2090d2c1 - Add tests that expose a problem with moving a block
Toggle commit list-
d1673b41...502c857b - 87 commits from branch
added 1 commit
- 8f811f7d - add new optional parameter in SectionListInterface::getSections to return sections keyed by uuid