[SPIKE] Prove that it's possible to apply block settings update paths to stored XB component trees

Created on 23 January 2025, 4 months ago

Overview

๐Ÿ“Œ Define `props` in the context of Block components Active is fixed, which means block settings can now be stored (and since ๐Ÿ“Œ Move SDC specific validation in ValidComponentTreeConstraintValidator::validate into the SDC source plugin Active are guaranteed to be valid).

Maintainers of block plugins can make changes to their config schema (to add/change/remove settings).

How does XB allow those maintainers to provide an update path that also updates all XB-stored component trees?

See search_post_update_block_with_empty_page_id(), tested by SearchBlockPageIdUpdatePathTest and added in ๐Ÿ“Œ Make Block config entities fully validatable Fixed for an example โ€” which ensures the search block's page_id setting is valid.

Proposed resolution

๐Ÿ“Œ Task
Status

Active

Version

0.0

Component

Data model

Created by

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @wim leers
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ
  • ๐Ÿ‡ช๐Ÿ‡ธSpain isholgueras

    Can we use ๐Ÿ“Œ Introduce unit test coverage for both ComponentSource plugins (Block + SDC) Active to use xb_test_block as blocks to update?

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom longwave UK

    1. We need to write an update path + update path test that does the exact same thing for a search block stored in an XB component tree.

    Are we thinking here that we need block implementations to provide separate code paths to update XB blocks? If so, what happens if a contrib author decides not to do that? Or, who takes responsibility for core block updates, if core does not depend on XB?

    On the other hand, I am not sure if this will work, but I wonder if we can do this without contrib maintainers needing to do anything at all! Block updates are generally written like this:

    function search_post_update_block_with_empty_page_id(&$sandbox = []) {
      $config_entity_updater = \Drupal::classResolver(ConfigEntityUpdater::class);
      $config_entity_updater->update($sandbox, 'block', function (BlockInterface $block): bool {
    ...
    

    Can we decorate ConfigEntityUpdater so as well as updating config entities, we additionally reconstruct config entities for blocks in XB content, and allow the closure to make changes to them in the same way? The closure is not responsible for saving the config entity - so because these won't be real config entities, we never need to directly save them; we can just extract the settings (or whatever else might have changed?) and put them back into the XB storage.

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    Will discuss #8 + #9 tomorrow with @longwave :)

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch

    I think that this is an existing core bug, because we should already be doing the same thing for layout builder and navigation modules, but we don't.

    Fixing it requires adding a new API to core, don't see another way, I opened ๐Ÿ› Block plugins need to be able to update their settings in multiple different storage implementations Active which doesn't have a nice API but I think probably does have the places that API would need to be implemented and roughly what it would need to do.

    I'm not postponing this issue on that one yet, but I think it probably should be.

    The sentence that got me to that point was this one from the parent issue:

    However โ€ฆ it can't actually work because we won't know which of those post-update hooks are associated with which block plugins! In theory, we could run all those block-targeting post-update hooks, but then we'd need to track for every single block-sourced component instance which update hooks have already been executed (equivalent to how core does the system-wide UpdateRegistry). That doesn't scale, just like it doesn't scale to update potentially millions of (revision) rows.

    This is the missing piece that the new BlockPluginUpdater API would address.

  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia larowlan ๐Ÿ‡ฆ๐Ÿ‡บ๐Ÿ.au GMT+10

    Given block updates would be writing config, can we instead listen to \Drupal\Core\Config\ConfigEvents::SAVE and detect if it occurs during an update/post update (assuming that's possible) and if so propagate those changes to component trees we can identify as referencing blocks via ๐Ÿ“Œ Calculate field and component dependencies on save and store them in an easy to retrieve format Active

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch

    I don't think it's possible to reliably detect whether the save happens during an update/post-update except perhaps via a backtrace and function name guessing (e.g. does post_update appear in the stack trace).

    Assuming that could be detected though, it would need to compare the before and after of the block entity, determine what the change is, and then apply that to the update - it wouldn't be possible to run the same code that the update itself does, only to reverse-engineer the change from the config entity original.

    Some block post updates won't result in any config saves at all, because there's no affected blocks on the site, but the plugins being targeted could be used in XB templates - then there's nothing to listen to. The search block would be an example where it might be in an XB template but not used in a block region.

    Additionally, this approach wouldn't be able to take into account xb templates that are shipped as config with modules or recipes, which equally need to apply the same changes so that they're correct after import/recipe apply.

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    Thanks to ๐Ÿ“Œ Calculate field and component dependencies on save and store them in an easy to retrieve format Active + ๐Ÿ“Œ [PP-1] Evaluate storing XB field type's "deps_*" columns in separate table Active , it's now trivial to find all the content entity revisions in which a certain block plugin is used.

    But what's still missing is fundamental core support for generically updating the inputs expected by plugins that may be used in multiple contexts, and not just e.g. Block config entities. The core issue for that is ๐Ÿ› Block plugins need to be able to update their settings in multiple different storage implementations Active .

    Thanks, @catch, for opening that, and stating it as starkly as you did: โ€” I don't either. ๐Ÿ˜ฌ

    Self-assigned in #10, forgot to link here to the write-up I posted at #3520484-22: [META] Production-ready ComponentSource plugins โ†’ as promised. Unassigning.

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    Once ๐Ÿ“Œ [PP-1] Consider not storing the ComponentTreeStructure data type as a JSON blob Postponed is in, I think we should be able to create a working PoC here that does the following in a test:

    1. Create a new input_schema_change_poc block plugin in the xb_test_block module which has e.g. ['foo' => 'bar'] as its default configuration, and has this as its config schema:
      block.settings.input_schema_change_poc:
        type: block_settings
        mapping:
          foo:
            type: string
            constraints:
              Choice: [bar, baz]
        constraints:
          FullyValidatable: ~
      
    2. Creates a component tree using that
    3. Installs a xb_test_block_simulate_input_schema_change module that:
      • overrides which class is used for that block plugin, and changes the default configuration to ['foo' => 'bar', 'change' => 'is scary'], plus changes its render logic in ::build() so we can easily assert the difference
      • alters the config schema to be:
        block.settings.input_schema_change_poc:
          type: block_settings
          mapping:
            foo:
              type: string
              constraints:
                Choice: [bar, baz]
            change:
              type: string
              constraints:
                Choice: ['is scary', 'is necessary']
          constraints:
            FullyValidatable: ~
        
    4. Make the test verify that the existing component instance renders the message provided by RenderSafeComponentContainer
    5. Then write manual logic because ๐Ÿ› Block plugins need to be able to update their settings in multiple different storage implementations Active does not exist yet that:
      • uses \Drupal\experience_builder\Audit\ComponentAudit to find all instances, in both config and content
      • update all instances of block.xb_test_block_simulate_input_schema_change in all component trees across both content and config
      • verify all those entities are again/still valid
      • verify that all those component trees render correctly again ๐Ÿ‘

    ๐Ÿ‘† That would allow us to be as confident as we can that we'll be able to do ๐Ÿ“Œ Component Source plugins: support for schema changes of explicit inputs Postponed in the refactored server-side data model ๐Ÿ‘

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    Just did a video call with @isholgueras to unblock him on next steps and clarify the unclear bits in my outline at #15 (๐Ÿ™ˆ Sorry!).

    Updated issue title + summary to convey the intended scope.

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    Explain relation to SDC and code components.

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    Fix HTML ๐Ÿ™ˆ

  • ๐Ÿ‡ช๐Ÿ‡ธSpain isholgueras
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    Reviewed and โ€ฆ nice progress here! ๐Ÿ‘๐Ÿ˜„

    Steps 4 and 5 in the plan I articulated in #15 are not yet done.

    Step 4 is not yet done due to a bug in the test coverage, once that's fixed, that'll force #4 ๐Ÿ˜‡

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    FYI: ๐Ÿ“Œ Version component prop definitions for SDC and Code components Active just landed, and ideally the test coverage for #15.5 would live in ComponentInputsEvolutionTest. That already contains testStorablePropShapeChanges(), this MR would add testInputSchemaChanges(). ๐Ÿ™

  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia larowlan ๐Ÿ‡ฆ๐Ÿ‡บ๐Ÿ.au GMT+10

    Late to the party but #15 sounds good to me.

  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia larowlan ๐Ÿ‡ฆ๐Ÿ‡บ๐Ÿ.au GMT+10
  • ๐Ÿ‡ช๐Ÿ‡ธSpain isholgueras
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    @larowlan in #26: thanks, much appreciated! ๐Ÿ™

    I kicked off a deep review and pushed a few small fixes/improvements which revealed test weaknesses, @f.mazeikis will continue ๐Ÿ‘

    P.S.: The 3 new test methods added to BlockComponentTest should be moved as I described in #24. ๐Ÿ™ This observation underlines that.

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom f.mazeikis Brighton

    f.mazeikis โ†’ made their first commit to this issueโ€™s fork.

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom f.mazeikis Brighton

    Between @isholgueras addressing some of your feedback and me moving tests out of BlockComponentTest into ComponentInputsEvolutionTest I think this is ready for another review.
    I have also taken liberty to rename test methods and expand their doc blocks, in attempts to make this PoC easier to read.

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ
Production build 0.71.5 2024