Make View blocks (block.settings.view_block:*) fully validatable

I spoke slightly too soon 😅

The update path should be moving none to null?

Update path is written, but still needs a test.

Blocking this on 📌 Make menu blocks (block.settings.system_menu_block:*) fully validatable Active , which makes a change to the BlockSettings migration plugin that will benefit this issue. (The change is complicated enough that it requires several tests to be adjusted, so I'd rather postpone than cross-port.)

Just want to note; we do have AtLeastOneOfValidator nowadays, although cleaning up is good, just want to. mention it.

📌 Make menu blocks (block.settings.system_menu_block:*) fully validatable Active is RTBC and unlikely to need any further changes before commit, so I'll proceed with this issue and base it in what's in there.

Update path test written! This should still probably be committed after 📌 Make menu blocks (block.settings.system_menu_block:*) fully validatable Active , but it's reviewable now.

Same question as for the other issue about the update hook, plus some other questions.

This also needs a change record as someone somewhere might be relying on that "none" string.

Change record written: https://www.drupal.org/node/3522240 →

Looking good, left some small comments.

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

Merged in changes from 11.x.

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

#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 .

#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.

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.

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.

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.

Added a question about historical string values.

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.

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!

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.

Added the presave hook and wrote a test. 🚀

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

Committed to 11.x - thanks!

larowlan → closed merge request !11932