Support third party settings for components within a section

The Needs Review Queue Bot → tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Patch re-rolled for Drupal 9.5.3

Something went wrong in some of the recent of the patches for 9.x. The patch adds use for classes that are declared already since a few Drupal core releases.

use Drupal\Core\Config\Entity\ConfigEntityUpdater;
use Drupal\Core\Entity\Display\EntityViewDisplayInterface;
use Drupal\layout_builder\Entity\LayoutEntityDisplayInterface;

use Drupal\Core\Config\Entity\ConfigEntityUpdater;
use Drupal\Core\Entity\Display\EntityViewDisplayInterface;
use Drupal\layout_builder\Entity\LayoutEntityDisplayInterface;
use Drupal\layout_builder\SectionComponent;

Another stab at re-roll...

Against 9.5.x

Against 10.1.x. Had conflicts on dictionary.txt

The last submitted patch, 192: 3015152-192.patch, failed testing. View results →

I feel that the migration from additional properties to third party settings needs to be benchmarked with as much scrutiny as the community can provide. Deprecating $additional is not something to take lightly for huge sites.

Here's a (hopefully, exaggerated) persona that might be able to help...

As a site owner with hundreds of content editors making dozens of updates to layout overrides per minute, and with 50,000+ layout overrides and 1,000,000+ revisions, the migration from additional properties to third party settings should incur no downtime or editorial confusion while the migration runs. Assuming the migration takes several hours, losing database connectivity or infrastructure resource problems should not cause a "we need to roll back" condition.

Patch #192 was fixed for failure while patch application, due to test case file changes DefaultsSectionStorageTest.php, in RC1 release. This patch is applicable for version 10.1.x-dev.

Re #196: I don't see where the there is currently an upgrade path for overrides. That would be up to each contrib or custom module to build into its handling of the deprecation. If custom_lb.module adds some $additional items, I would expect/hope that it would handle the deprecation.

Re-roll for 11.x and fixing version strings in deprecations.

Re #197, see layout_builder_component_attributes_uninstall() in the Layout Builder Component Attributes → modules. My plan is to adapt that code to move settings in $additional to third-party settings. It covers entity view displays, as well as overrides (including all revisions).

That said, it hasn't been tested as contemplated in #195. I'm happy to collaborate on that point.

The last submitted patch, 198: 3015152-198.patch, failed testing. View results →

No interdiff on the re-roll. The only conflicts where in cspell and were trivial to resolve manually.

CC failures, but #198 appeared to have some test failures.

I noticed the code quality warnings in the MR. Nice! I fixed the ones I could figure out off the top of my head how to fix. The others I'm not sure how to deal with. While we are not removing the $additional property, are we doing anything to convert the incoming attributes to third party settings? Need someone who understands this better to weigh in.

Just a note - the latest state of MR seems to be applying cleanly to 10.2.x (which isn't the case with patches above it).

I have some performance details on mass update hooks to layout overrides. Over in the issue to add UUIDs to sections, updating 50,000 layout overrides can put a site into maintenance mode for a whopping approximately 15 minutes.

For stakeholders, that means downtime. $.02

Could someone make a patch for drupal 10.2 version? Is there possibility that this patch comes for drupal 10.2 and possibly also compability for php 8.2? With 8.2 php there is a php info in logs:

Deprecated function: Creation of dynamic property Drupal\layout_builder\SectionComponent::$legacyAdditionalModuleKey is deprecated in Drupal\Core\Entity\Sql\SqlContentEntityStorage->loadFromDedicatedTables() (line 1269 of /web/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php)

@ActuallyAM it gets confusing when there are patches posted along with MRs, but you can create a patch for your own testing. Find the MR you need at the top of this issue, then click the link for Plain Diff, and save as a .patch file.

Patch of the latest MR like capysara suggested

This is a reroll of #211 for 10.3.x

Applying this patch from #213 was helpful in make layout_builder_styles work properly when upgrading to 10.3.1.

I was getting

Deprecated function: Creation of dynamic property Drupal\layout_builder\SectionComponent::$thirdPartySettings is deprecated in Drupal\Component\Serialization\PhpSerialize::decode() (line 21 of core/lib/Drupal/Component/Serialization/PhpSerialize.php).

and also

Error: Call to a member function getSection() on null in Drupal\layout_builder\Form\ConfigureBlockFormBase->getCurrentSection() (line 341 of /var/www/web/core/modules/layout_builder/src/Form/ConfigureBlockFormBase.php).

When trying to create a new block or edit an existing block.

One note: the patches from MR 8474 or comment #211 work and solve the problem. However, a PHP 8.2 deprecation notice remains as stated on #209:

Deprecated function: Creation of dynamic property Drupal\layout_builder\SectionComponent::$legacyAdditionalModuleKey is deprecated in Drupal\Core\Entity\Sql\SqlContentEntityStorage->loadFromDedicatedTables() (line 1269 of /web/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php)

Drupal 10.3 and PHP 8.2 compatibility MR has been created: MR 8724. Can someone please review?

acbramley → changed the visibility of the branch 10.2.x to hidden.

acbramley → changed the visibility of the branch 11.x to hidden.

We have 3 patches, 8 branches, and 6 MRs on this issue. That's basically impossible for anyone to review or know what to contribute to. Can we please get these cleaned up and documented in the IS?

None of the branches (diff files) or patches seem to apply to Drupal 11.0.5

Here is a patch for Drupal 11.0.5.

Update, I think I got it how to create a patch for 11.0.5 with minimal effort.
- checkout 11.0.5 in a clean Drupal repo clone:
git checkout tags/11.0.5
- copy the (incompatible) 11.x diff file in the root (5053.diff)
- use this command to apply all the parts the patch that do work:
git apply --reject --index 5053.diff
- Notice that there is only 1 error: error: core/misc/cspell/dictionary.txt: does not match index
- manually edit dictionary.txt with the necessary changes (add 4 missing words in the right place)
- remove the 5053.diff file
- create a patch
git diff > 3015152-225.patch
I think it might work.

acbramley → changed the visibility of the branch drupal-3015152-3015152-support-third-party-11.x to hidden.

acbramley → changed the visibility of the branch drupal-3015152 to hidden.

acbramley → changed the visibility of the branch 3015152-support-third-party to hidden.

acbramley → changed the visibility of the branch 3015152-support-third-party-9.4.x to hidden.

acbramley → changed the visibility of the branch 3015152-support-third-party-10.2.x to hidden.

Attempting to consolidate MRs to make this issue manageable and hiding all patches

Work must go into 11.x first and then it will be backported to 10.3.x if possible, adding multiple MRs to this issue is just making it harder to contribute to and review. Please continue working on https://git.drupalcode.org/project/drupal/-/merge_requests/5053/diffs as this seems to be the most up to date version from what I can see.

acbramley → changed the visibility of the branch 3015152-support-third-party-10.3.x-php8.2 to hidden.

Update tests added and are green now. IS seems up to date. Ready for review.

If I use MR 5053 on Drupal 11.1.1 I get multiple notices on each page:
Deprecated: Creation of dynamic property Drupal\layout_builder\SectionComponent::$legacyAdditionalModuleKey is deprecated in /var/www/html/web/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php on line 1264

If I try to find the origin of these errors, I edited web/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php and added dpm($row->$column_name); right before line 1263. Result is in attached schreenshot. The value of $column_name is layout_builder__layout_section. So I guess this is the serialized data of an existing layoutbuilder section that was no problem at all when the project was on D10 (including diff from this issue) but is a problem now when the project is updates to D11 (including diff from this issue).

I have no idea on how to remove this data from existing sections. I cannot create edit or save sections or blocks using layoutbuilder becasue that too shows these errors.

@flyke thanks for that, I was investigating how to remove this key myself for quite a while and came up short. The main issue I was running into is that when you save the node, the LayoutSectionItemList::equals was stopping the old data from being overwritten because toArray drops the key, therefore it doesn't save. However, since you're creating a new component (and therefore new UUID) it should work!

You also need to do this for every revision, otherwise you get the same errors on the revisions page as well.

A bit of a relief to know I'm not the only one facing this problem. I did spent quite a lot of time figuring this out and coming up with a woking solution. Glad that it has helped someone. Maybe someone could use my code as a starting point to make it more elegant/efficient and include revisions.

Since my custom code to get all layoutbuilder nodes as only 2 lines, I will update my code examply from above so everyone could copy/paste it.

Thanks so much @flyke, I've created a gist that improves upon your solution a bit with the following:

1. Makes it a batched process to avoid memory issues
2. Filters the initial query so only revisions with that key are loaded
3. Generates a new UUID for the new section, I was having issues where some components were being dropped off without this.

This uses an entity migration helper that we often use for these kind of operations, it's flexible enough to apply to any sort of data migration involved with entities :)

https://gist.github.com/acbramley/53578b18b27466f1b508aedb12907b5e

I've added this to the IS.

acbramley → changed the visibility of the branch 3015152-support-third-party-10.3.x to hidden.

Hi @acbramley I don't understand how to use your method, more specifically, how to run it.
I have an enabled custom module 'starterkit_helper'.
In there, I created starterkit_helper.post_update.php file. The contents from the first file of your gist.
Of course inside i changed 'my_profile' to 'starterkit_helper'.
I also created a file starterkit_helper/src/MyProfileUpdateUtils.php. The contents from the second file of your gist.
Of course inside i changed 'my_profile' to 'starterkit_helper'.

I ran drush cr and then drush updb -y but nothing happened, nothing run, en the errors
Deprecated : Creation of dynamic property Drupal\layout_builder\SectionComponent::$legacyAdditionalModuleKey is deprecated in /var/www/html/web/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php on line 1264
remain

How do I run this code ?

Hey @flyke those steps seem correct to me, the only thing would be perhaps if you had the post update function in code before you enabled the module, Drupal may have marked that as already run. Try renaming the function and running drush updb again.

I wonder if the gist in #239 could be optimized by operating on the layout builder section storage tables and bypassing the Entity API? The SectionStorage can be loaded and saved independently of the content entity it's attached to I believe.

Generally, saving content entities is MAJORLY slow comparatively. I plan on running additional benchmarking on this at some point as we have over a quarter million sections to churn through when this finally lands. 🤞

Feel free to optimise away @luke.leber :) We only had about 1700 revisions to get through. Yes it's slower but this was the easiest approach for us without sinking too much time.

Feel free to review, but this approach seems very reasonable to me in terms of performance 💪. This operates at a very low level, so it's very likely that this first stab is not 100% perfect.

Business logic encapsulated in a helper service

<?php

namespace Drupal\psu_layout_builder;

use Drupal\Core\Database\Connection;
use Drupal\Core\StringTranslation\TranslatableMarkup;
use Drupal\layout_builder\Section;
use Drupal\layout_builder\SectionComponent;

/**
 * Helper to facilitate 'additional' to 'third_party_settings' migration.
 */
class SectionComponentTPS {

  /**
   * Service constructor.
   *
   * @param \Drupal\Core\Database\Connection $database
   *   The database connection service.
   */
  public function __construct(
    protected Connection $database
  ) { }

  /**
   * Migrates 'additional' to 'third_party_settings' for the provided table.
   *
   * @param string $table
   *   The database table to migrate.
   * @param array $sandbox
   *   The post-update sandbox.
   *
   * @throws \Exception
   *   😱 God help you if this happens.
   */
  public function migrate(string $table, array &$sandbox, $sections_per_batch = 10000) {
    if (!isset($sandbox['total'])) {
      $query = $this->database->select($table);
      $sandbox['total'] = $query->countQuery()->execute()->fetchObject()->expression;
      $sandbox['current'] = 0;
      if (empty($sandbox['total'])) {
        $sandbox['#finished'] = 1;
        return NULL;
      }
    }

    $query = $this->database
      ->select($table, 'layout')
      ->fields('layout', ['bundle', 'entity_id', 'revision_id', 'langcode', 'delta', 'layout_builder__layout_section'])
      ->range($sandbox['current'], $sections_per_batch);

    $rows = $query->execute()->fetchAll(\PDO::FETCH_ASSOC);
    if (empty($rows)) {
      $sandbox['#finished'] = 1;
      return;
    }
    else {

      // Combine all update queries into a single upsert for performance.
      $upsert = $this->database->upsert($table)
        ->fields(['bundle', 'entity_id', 'revision_id', 'langcode', 'delta', 'layout_builder__layout_section'])
        ->key('revision_id');

      foreach ($rows as $row) {
        /** @var \Drupal\layout_builder\Section $section */
        $section = unserialize($row['layout_builder__layout_section'], ['allowed_classes' => [Section::class, SectionComponent::class]]);
        foreach ($section->getComponents() as $component) {
          // Set might be deprecated, but it's hella efficient.
          // Don't run this code on Drupal 12, eh?
          $component->set('thirdPartySettings', $component->get('additional') ?: []);
          $component->set('additional', []);
        }
        $row['layout_builder__layout_section'] = serialize($section);
        $sandbox['current']++;
        $upsert->values($row);
      }

      $upsert->execute();
    }

    if ($sandbox['current'] >= $sandbox['total']) {
      $sandbox['#finished'] = 1;
    }
    else {
      $sandbox['#finished'] = ($sandbox['current'] / $sandbox['total']);
    }
    return new TranslatableMarkup('@count of @total' . ' layout sections processed.', [
      '@count' => $sandbox['current'],
      '@total' => $sandbox['total'],
    ]);
  }

}

Service configuration boilerplate

services:
  Drupal\psu_layout_builder\SectionComponentTPS:
    class: Drupal\psu_layout_builder\SectionComponentTPS
    autowire: true

Sample post update hooks for *one* entity type

/**
 * Migrate 'additional' to 'third_party_settings' for nodes.
 */
function psu_layout_builder_post_update_tps_nodes(array &$sandbox) {
  return \Drupal::service(SectionComponentTPS::class)->migrate('node__layout_builder__layout', $sandbox, 10000);
}

/**
 * Migrate 'additional' to 'third_party_settings' for node revisions.
 */
function psu_layout_builder_post_update_tps_node_revisions(array &$sandbox) {
  return \Drupal::service(SectionComponentTPS::class)->migrate('node_revision__layout_builder__layout', $sandbox, 10000);
}

Let's do this thing @ 10,000 records per batch.

www-data@9218f372c877:~/html$ time drush updb
 -------------------- -------------------- ------------- ---------------------- 
  Module               Update ID            Type          Description           
 -------------------- -------------------- ------------- ---------------------- 
  psu_layout_builder   tps_node_revisions   post-update   Migrate 'additional'  
                                                          to                    
                                                          'third_party_setting  
                                                          s' for node           
                                                          revisions.            
  psu_layout_builder   tps_nodes            post-update   Migrate 'additional'  
                                                          to                    
                                                          'third_party_setting  
                                                          s' for nodes.         
 -------------------- -------------------- ------------- ---------------------- 


 Do you wish to run the specified pending updates? (yes/no) [yes]:
 > 

>  [notice] Update started: psu_layout_builder_post_update_tps_node_revisions
>  [notice] 10000 of 208192 layout sections processed.
>  [notice] 20000 of 208192 layout sections processed.
>  [notice] 30000 of 208192 layout sections processed.
>  [notice] 40000 of 208192 layout sections processed.
>  [notice] 50000 of 208192 layout sections processed.
>  [notice] 60000 of 208192 layout sections processed.
>  [notice] 70000 of 208192 layout sections processed.
>  [notice] 80000 of 208192 layout sections processed.
>  [notice] 90000 of 208192 layout sections processed.
>  [notice] 100000 of 208192 layout sections processed.
>  [notice] 110000 of 208192 layout sections processed.
>  [notice] 120000 of 208192 layout sections processed.
>  [notice] 130000 of 208192 layout sections processed.
>  [notice] 140000 of 208192 layout sections processed.
>  [notice] 150000 of 208192 layout sections processed.
>  [notice] 160000 of 208192 layout sections processed.
>  [notice] 170000 of 208192 layout sections processed.
>  [notice] 180000 of 208192 layout sections processed.
>  [notice] 190000 of 208192 layout sections processed.
>  [notice] 200000 of 208192 layout sections processed.
>  [notice] 208192 of 208192 layout sections processed.
>  [notice] Update completed: psu_layout_builder_post_update_tps_node_revisions
>  [notice] Update started: psu_layout_builder_post_update_tps_nodes
>  [notice] 6082 of 6082 layout sections processed.
>  [notice] Update completed: psu_layout_builder_post_update_tps_nodes
 [success] Finished performing updates.

real    0m15.490s
user    0m7.965s
sys     0m0.749s

15 seconds to churn through over 210,000 records seems agreeable to me!

Not sure what I'm doing wrong, but @luke.leber's method executed without errors but it did not solve anything in my project.
On the left side of the screenshot you can see in the terminal below that it executed the 2 updates.
On the right side of the screenshot: that is refreshed after the updates were executed. You can see the errors are still there in my case.

After this, I tried my own code from #236 and that actually fixed and removed all the errors.

Appears to need a rebase

Can the CR be updated though it doesn't really seem to line up with the MR or explain the change to me

If you are another contributor eager to jump in, please allow the previous poster at least 48 hours to respond to feedback first, so they have the opportunity to finish what they started!

Reroll for 11.x

@smustgrave anything in particular you're unsure of in the CR? I've modified the title slightly and updated versions

I think the CR reads fine now and cleared it up for me. Fingers crossed.

Thanks Luke for providing the example migration code. I agree that manually unserializing, reserializing, and saving the data directly to the database bypassing the entity API is the only sane way to do an upgrade path for layout overrides. We run 1,000 sites with what I imagine contain hundreds of thousands of overrides across them (including revisions). Entity load/save will simply take too long.

I hate to do this, but I'm setting this back to Needs Work to at least have the CR updated to provide the critical information that modules that were relying on "additional" need to write their own upgrade path for layout overrides to switch TPS. I say this as a maintainer of the Layout Builder Styles module which critically relies on additional. Also, given that the upgrade path should essentially be identical for all contrib modules, perhaps it should actually be included in the MR. The current upgrade path in this MR only covers the default entity view displays.

I also want to throw this out there: What if we just don't deprecate additional at all? Just leave it there and discourage it's use in comments and point people at TPS instead. I'm thinking of all the hours of time developers and module maintainers will need to spend on this upgrade path and it hurts to think about.

@bkosborne I understand the concern but I don't think we can automate that upgrade path from core. We aren't removing additional, we're deprecating it so modules will be aware they need to change. How would we know how modules are using those additional settings? If they are doing $component->get('foo'); and we're automatically moving additional -> tps then that code would suddenly be broken, they need to change that to getThirdPartySetting.

re #253/254: I came to the exact same conclusion as Adam after I had to convert some custom code to using tps. It was very specific to that custom module. I also ended up writing the upgrade path as a BC compliant solution so it would convert on page load. Of course, that is all very custom to that site's use case. There could be a hundred ways to convert over.

Secondly, is there any value in converting? Yes. If you ever tried to enforce config schema on additional vs tps, you'll immediately see that its impossible with additional. I've been waiting for tps on sections for several years for just that reason. Sure it will be painful. Arguably though we shouldn't have use additional in the first place. But we live and learn.

As the maintainer for the Layout Builder Component Attributes modules, who also has contributed code to this issue, my vote is to commit. It'll be up to contrib to handle upgrade paths. The code provided by @luke.leber is what I'm planning on using. (@luke.leber THANK YOU for sharing that code btw.) It's performant and can handle a large number of records. Modules using the additional property will have until D12 to address the deprecation.

I would like to be linked to one other core commit that would force site owners to run multiple content updates on every single revision of every single content entity on their site.

In the past 10 years I cannot think of one. IMO this sets a very dangerous precedent..

What happens if MYSQL goes away mid-update? Race conditions? Many known race conditions exist in core.

Thanks for the valuable feedback everyone, I think this can go back to RTBC then.

#257: I don't think you technically _must_ create a hook_update. You could just keep legacy code, check a Settings.php key or State key or something to see if you should trigger the legacy `additional` logic. You could also write a cron queue job that processes revisions nightly between 10pm and 4am. There are lots of options here. Performance of running the update isn't the biggest concern to me. I've added a link in the CR to comment #245 who might that helpful.

For other places where this happened, look at the Group v2/v3 => v4 upgrade path. Its a bit gnarly but seems to work.

Atlanta would be a great space to discuss this. Between LB styles, LB component attributes, and who knows what else in contrib space, how would contrib even coordinate all of these upgrade paths? Would every module do it independently? Only for their third party settings? What about custom stuff too?

I could see some sites needing like 4 or 5 iterations of saving every revision to make this work in contrib/custom...it just seems like a very messy thing for core to put the burden on site owners with.

If it's not an easy upgrade (read: something that non-technical site owners would have to spend $$$ on an agency to custom code a migration for), odds are trust will be damaged. In today's super competitive market, damaging trust shouldn't be downplayed -- especially if migrating to a competitor's solution ends up being cheaper to the business.

$.02 😥