Skip to content
Snippets Groups Projects

Issue #3208766: Add UUID to sections

Merge request reports

Members who can merge are allowed to add commits.
Code Quality is loading
Test summary results are being parsed
Ready to merge by members who can write to the target branch.
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
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 Sachdev
  • Kunal Sachdev changed this line in version 23 of the diff

    changed 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 Sachdev
  • Ok, 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
  • Please register or sign in to reply
  • Kunal Sachdev added 25 commits

    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()...

    Compare with previous version

  • Kunal Sachdev added 1 commit

    added 1 commit

    • 16c43a03 - re-key every section in sections array after removing a section| fix some tests

    Compare with previous version

  • Kunal Sachdev added 1 commit

    added 1 commit

    Compare with previous version

  • Kunal Sachdev added 1 commit

    added 1 commit

    • e2e66a1f - fix core.entity_view_display.entity_test.bundle_with_extra_fields.default.yml

    Compare with previous version

  • Kunal Sachdev added 1 commit

    added 1 commit

    • f516f222 - new method SectionListInterface::getSectionsByUuid() and its test

    Compare with previous version

  • Kunal Sachdev added 1 commit

    added 1 commit

    Compare with previous version

  • 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 :smile:🤯

      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 Leber
    • Yep, that means that if this update hook does not use the batch system, that anybody not updating using drush but through the UI (/update.php) will run into PHP's max execution time!

    • Kunal Sachdev changed this line in version 30 of the diff

      changed this line in version 30 of the diff

    • Please register or sign in to reply
  • Sean Adams-Hiett
    Sean Adams-Hiett @pyrello started a thread on an outdated change in commit 6fcadc37
  • 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.

    • This is actually reflected in the changes to the demo_umami config you also did in this commit. Those need to be keyed by UUID to be able to compare them with config tools.

    • Ok, I deleted the latest commits which changed it.

    • Please register or sign in to reply
  • Kunal Sachdev added 1 commit

    added 1 commit

    • 6ca1a08f - use batch system in update hook

    Compare with previous version

  • Kunal Sachdev added 1 commit

    added 1 commit

    • 0a253ef7 - Add BC for SectionListInterface methods

    Compare with previous version

  • Kunal Sachdev added 1 commit

    added 1 commit

    Compare with previous version

  • Kunal Sachdev added 1 commit

    added 1 commit

    • 420b0bdb - fix methods in LayoutBuilderEntityViewDisplay

    Compare with previous version

  • Kunal Sachdev added 1 commit

    added 1 commit

    • b4c4dabb - more changes in LayoutBuilderEntityViewDisplay

    Compare with previous version

  • Kunal Sachdev added 1 commit

    added 1 commit

    • fbbb5afa - fix layout_builder_install()

    Compare with previous version

  • Kunal Sachdev added 1 commit

    added 1 commit

    • 70a5e3da - new method SectionListInterface::getSectionByDelta($delta) and use it instead...

    Compare with previous version

  • Kunal Sachdev added 1 commit

    added 1 commit

    • ac4fd9ca - test changes work-in-progress

    Compare with previous version

  • Kunal Sachdev added 1 commit

    added 1 commit

    • 094c2d52 - more test fixes and new test coverage

    Compare with previous version

  • Kunal Sachdev added 1 commit

    added 1 commit

    • ebaabe19 - more improvements and test fixes

    Compare with previous version

  • Kunal Sachdev added 1 commit

    added 1 commit

    • d1673b41 - dont use strict checking for delta because sometimes its type is string

    Compare with previous version

  • 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) {
  • 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));
  • 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));
  • Tim Plunkett added 113 commits

    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

    Compare with previous version

  • Kunal Sachdev added 1 commit

    added 1 commit

    • 8f811f7d - add new optional parameter in SectionListInterface::getSections to return sections keyed by uuid

    Compare with previous version

  • Kunal Sachdev added 1 commit

    added 1 commit

    Compare with previous version

  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Please register or sign in to reply
    Loading