Closed (fixed)
Project:
Drupal core
Version:
11.x-dev
Component:
views.module
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
24 Apr 2025 at 10:53 UTC
Updated:
22 May 2025 at 22:04 UTC
Jump to comment: Most recent
Problem/Motivation
#3379725: Make Block config entities fully validatable made Block config entities fully validatable, and the settings for 2 block plugins:
- content blocks (
block.settings.block_content:*:) (seeblock_content.schema.yml - the search form block
block.settings.search_form_block(seesearch.schema.yml)
Steps to reproduce
N/A
Proposed resolution
Also make
views_block:
type: block_settings
label: 'View block'
mapping:
views_label:
type: label
label: 'Title'
items_per_page:
type: string
label: 'Items per block'
block.settings.views_block:*:
type: views_block
fully validatable.
Confusingly:
views_labelis made available by\Drupal\views\Plugin\Block\ViewsBlockBase::buildConfigurationForm()items_per_pageis made available by\Drupal\views\Plugin\views\display\Block::blockForm()
The two are brought together into a single form by a third method: \Drupal\views\Plugin\Block\ViewsBlock::blockForm()
So … this seems surprisingly simple? 😄 All the actual control over the block lives in the associated View entity, not in the Views Block's configuration! Lucky us: no need for an update path! 😄
One complication:
case 'items_per_page':
$form['override']['items_per_page'] = [
'#type' => 'select',
'#title' => $this->t('Items per block'),
'#options' => [
'none' => $this->t('@count (default setting)', ['@count' => $this->getPlugin('pager')->getItemsPerPage()]),
1 => 1,
2 => 2,
3 => 3,
4 => 4,
5 => 5,
6 => 6,
10 => 10,
12 => 12,
20 => 20,
24 => 24,
40 => 40,
48 => 48,
],
'#default_value' => $block_configuration['items_per_page'],
];
… which is why it's stored as type: string: it's either none, OR an integer >=1. We should change that to something more sensible:
type: integer
# In case you want to inherit the default
nullable: true
constraints:
Range:
min: 1
Remaining tasks
- Add the missing config schema.
- Review.
User interface changes
None.
Introduced terminology
None.
API changes
None.
Data model changes
TBD
Release notes snippet
TBD
Issue fork drupal-3520946
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
- 3520946-views-blocks-fully-validatable
changes, plain diff MR !11932
Comments
Comment #2
wim leersI spoke slightly too soon 😅
Comment #4
borisson_The update path should be moving
nonetonull?Comment #5
quietone commentedComment #7
phenaproximaUpdate path is written, but still needs a test.
Comment #8
phenaproximaBlocking this on #3520944: Make menu blocks (block.settings.system_menu_block:*) fully validatable, 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.)
Comment #9
bbralaJust want to note; we do have
AtLeastOneOfValidatornowadays, although cleaning up is good, just want to. mention it.Comment #10
phenaproxima#3520944: Make menu blocks (block.settings.system_menu_block:*) fully validatable 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.
Comment #11
phenaproximaUpdate path test written! This should still probably be committed after #3520944: Make menu blocks (block.settings.system_menu_block:*) fully validatable, but it's reviewable now.
Comment #12
longwaveSame question as for the other issue about the update hook, plus some other questions.
Comment #13
longwaveThis also needs a change record as someone somewhere might be relying on that "none" string.
Comment #14
phenaproximaChange record written: https://www.drupal.org/node/3522240
Comment #15
phenaproximaComment #16
bbralaLooking good, left some small comments.
Comment #17
longwave#3520944: Make menu blocks (block.settings.system_menu_block:*) fully validatable landed so this needs rebase.
Comment #18
phenaproximaMerged in changes from 11.x.
Comment #19
phenaproximaDamn 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 #3520944: Make menu blocks (block.settings.system_menu_block:*) fully validatable but I guess that can be handled in a follow-up during beta.)
Comment #20
wim leers#19: the Layout Builder concern is a known critical core bug: #3521221: Block plugins need to be able to update their settings in multiple different storage implementations.
Comment #21
phenaproximaComment #22
catchIt'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.
Comment #23
catchProbably 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.
Comment #24
wim leersTrue. 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_pagesetting 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.
Comment #25
wim leersMissing 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.
Comment #26
longwaveAdded a question about historical string values.
Comment #27
longwaveIt'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.
Comment #28
wim leersIsn'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!
Comment #29
larowlanI 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::blockPresavefor #3379725: Make Block config entities fully validatable so I think we should do the same here.Comment #30
phenaproximaAdded the presave hook and wrote a test. 🚀
Comment #31
penyaskitoReviewed 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:
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
Comment #33
larowlanCommitted to 11.x - thanks!