#Experience Builder

larowlan → closed merge request !11932

Committed to 11.x - thanks!

Reviewed this, only a small nit (with a suggestion) which can be ignored at discretion of committers.

As I was only partially aware of the issue and didn't participate before, I read:

• Every comment here.
• Every resolved comment in the MR.
• https://git.drupalcode.org/project/drupal/-/merge_requests/6938/diffs for having context of a similar issue.
• Follow-ups/TODOs (nice that we have an issue for every of them!)
  • 🐛 Block plugins need to be able to update their settings in multiple different storage implementations Active
  • 📌 [PP-1] Deprecate and remove `status` and `info` settings from `block_content` blocks Postponed
• The change record: https://www.drupal.org/node/3522240 →

I didn't manually test this, but test coverage covers everything I could think of, and others have done before.

Honestly surprised and amazed of having more config schema validation for views!

Happy to see metrics up, one less to go! https://git.drupalcode.org/issue/drupal-3379725/-/jobs/2197356

Added the presave hook and wrote a test. 🚀

I think we need a hook_block_presave here to handle triggering notifications for out of date exported default config.
We added one in \Drupal\search\Hook\SearchHooks::blockPresave for 📌 Make Block config entities fully validatable Fixed so I think we should do the same here.

It's somewhat disappointing how difficult these seemingly simple config schema changes are to actually carry out

Isn't that more the strong reliance on typecasting and absence of validation in Views? (At least in this particular case.)

Either way: yay, and thanks so much!

It's somewhat disappointing how difficult these seemingly simple config schema changes are to actually carry out, but I think this one is solid now, and the BC patterns and test coverage we figured out here can hopefully be reused in similar future issues.

Added a question about historical string values.

Missing test coverage added in https://git.drupalcode.org/project/drupal/-/merge_requests/11932/diffs?c....

That >fails tests, which shows @catch's concerns in #23 were justified: this would have broken views block instances in Layout Builder! 😨

Fixed in https://git.drupalcode.org/project/drupal/-/merge_requests/11932/diffs?c....

I think this is now ready for final review.

I don't think that we had many block plugin settings updates in the past so we got away with it, as far as we know anyway.

True. But that doesn't make it less of a bug/oversight? 😅

The change I proposed and which @phenaproxima implemented means we can actually revert most update path logic.

I just also pushed a commit that removes the need for the items_per_page setting to exist on all views blocks, which further reduces the amount of change.

But I agree with @catch's concerns in #23, and I disagree with @phenaproxima's — Views simply doesn't have sufficiently thorough tests to catch it. Plus … the logic was still checking for 'none' 😅.

So: this IMHO needs additional test coverage anyway, and if we're doing that, we might as well prove that #23 still continues to work.

Probably the main question with #22 is whether this actually causes a problem or not for LB, if it doesn't, then it's not making things worse, but that probably needs manual testing at least.

#19: the Layout Builder concern is a known critical core bug

It's a known bug now as of a couple of weeks ago, but it's only become known because there are lots of proposed updates to block settings in the queue, which previously there weren't. I don't think that we had many block plugin settings updates in the past so we got away with it, as far as we know anyway.

If we fix the bug in layout builder (and it's also a bug in experience builder, and the navigation module), and have already added these block module-specific update paths to core, we will need to go through all of those updates paths and refactor them to the new system, if that's possible. So I'm not really sure what to do here.

Gah, crosspost

@longwave the MR was closed but it doesn't look like this was committed/pushed - was that by mistake?

griffynh → credited meghasharma → .

griffynh → credited berdir → .

griffynh → credited danielveza → .

griffynh → credited kristen pol → .

griffynh → credited penyaskito → .

#19: the Layout Builder concern is a known critical core bug: 🐛 Block plugins need to be able to update their settings in multiple different storage implementations Active .

Damn it, I just realized that this update path is more complex than previously thought because Views blocks might be embedded in Layout Builder displays. (Same thing goes for 📌 Make menu blocks (block.settings.system_menu_block:*) fully validatable Active but I guess that can be handled in a follow-up during beta.)

Merged in changes from 11.x.

📌 Make menu blocks (block.settings.system_menu_block:*) fully validatable Active landed so this needs rebase.

longwave → closed merge request !11933

Committed df9baa9 and pushed to 11.x. Thanks!

Looking good, left some small comments.