The `label_display` setting in `type: block_settings` has the wrong config schema type

Migrating Pittsburgh 2023 to Pittsburgh2023 tag for cleanup

Tried re-rolling patch. Please review

Seems reroll caused failures. And should target 11.x

Added Reroll with respect to 11.x

Will let the tests finish but don't think the reroll was correct. Look at the file sizes. Almost tripled in size.

@smustgrave, Added reroll patch against 11.x with respect of #26, As Bartik and Seven theme are not the part of D10, 11, So below configurations are not part of this reroll, Please have a look and advise If this is a correct one.

core/profiles/standard/config/optional/block.block.bartik_account_menu.yml
core/profiles/standard/config/optional/block.block.bartik_breadcrumbs.yml
core/profiles/standard/config/optional/block.block.bartik_content.yml
core/profiles/standard/config/optional/block.block.bartik_footer.yml
core/profiles/standard/config/optional/block.block.bartik_help.yml
core/profiles/standard/config/optional/block.block.bartik_main_menu.yml
core/profiles/standard/config/optional/block.block.bartik_messages.yml
core/profiles/standard/config/optional/block.block.bartik_powered.yml
core/profiles/standard/config/optional/block.block.bartik_search.yml
core/profiles/standard/config/optional/block.block.bartik_tools.yml
core/profiles/standard/config/optional/block.block.seven_breadcrumbs.yml
core/profiles/standard/config/optional/block.block.seven_content.yml
core/profiles/standard/config/optional/block.block.seven_help.yml
core/profiles/standard/config/optional/block.block.seven_messages.yml

Seemed to have failures.

Rerolled the patch - note that umami profile and olivero adds weight to this change as there's a number of additional block configs that required updating.

Hiding patches as work has switched to MR 4729 which is on 11.x

Moving to NW for CC failure.

Am confused as the patch sizes have shifted each patch. So not sure if things were being removed or added.

Tagging for issue summary for what is being fixed.

@smustgrave stuff is being added to the merge request. Turns out there are a LOT of tests and config for blocks, lots of which set the label_display, hence the merge request growing.

Will update the issue summary asap!

Sorry about the mess above! I'm still learning this new system.

I've got most of this done now, but having a problem with some tests passing, which you can see here - https://git.drupalcode.org/issue/drupal-2544708/-/jobs/202425

Would welcome some advice on how to get those tests passing. I believe it will be because the block config in the config tarball will have the old schema but not 100% sure how to update that.

I'll put this into "needs review" so we can get some eyes on it but appreciate it's not quite ready!

Test fails because of 'type' issue, Try to fixed that.

The changes you made to MessageViewBuilder.php and LayoutBuilderEntityViewDisplay.php I'm pretty certain are not correct although the other two ()help.post_update.php and MigrateBlockTest.php) look to have been missed. The failing tests are update tests which need a correct update hook - the one I added isn't quite correct. Will be looking at this on Friday and hopefully will be able to get this passing all tests! :-)

Seems there's still work to be done.

@welly As mentioned in #49, reverted the file changes for 'MessageViewBuilder.php' & 'LayoutBuilderEntityViewDisplay.php'. During the investigation last I have added those, but now its reverted and try to fix the remaining tests.

Adding the configuration schema tag.

Have hit a bit of a wall with these tests.

Failed tests I'm coming up against are the following:

1. https://git.drupalcode.org/issue/drupal-2544708/-/jobs/381558#L1695

I'm struggling to see where the string type variable is coming from. When I put breakpoints into the code, I can see that the config value for core.entity_view_display.node.page.default:third_party_settings.layout_builder.sections.0.components.aeb0f8ae-7f1a-4d65-aaeb-c26ce56fa27d.configuration.label_display is set to "0". I just don't know where that is coming from.

It doesn't seem to be here: https://git.drupalcode.org/issue/drupal-2544708/-/blob/2544708-display-t...

2. https://git.drupalcode.org/issue/drupal-2544708/-/jobs/381558#L818

This seems to be unrelated to my PR so again, struggling to see how this is failing. If I check

core/tests/fixtures/config_install/multilingual.tar.gz there doesn't seem to be any deleted views configs so again, struggling to understand how this is failing.

3. https://git.drupalcode.org/issue/drupal-2544708/-/jobs/381557

All the tests are passing so not sure what the fail is here.

ERROR: Job failed: pod "runner-tdd8nzs6-project-119803-concurrent-1-w67bvbwo" status is "Failed"

And there are some failed tests in the Functional Javascript tests which I'll come back to as I've not looked at them too closely yet.

So any advice would be much appreciated!

Going to look at the modified fixtures and see if they need repairing....

Looks like there is still a failure in TimestampFormatterSettingsUpdateTest, but that is the only remaining failure, that should be fixed.

So I've spent a bit of time trying to resolve this TimestampFormatterSettingsUpdateTest error without success. When that test runs, the runUpdate method call in testPostUpdateTimestampFormatter fails with the following assertion failures

There should be no errors in configuration 'core.entity_view_display.node.page.default'. Errors:

Schema key core.entity_view_display.node.page.default:third_party_settings.layout_builder.sections.0.components.b961467b-96b6-47da-baab-fec95554f963.configuration.label_display failed with: variable type is string but applied schema class is Drupal\Core\TypedData\Plugin\DataType\BooleanData
Schema key core.entity_view_display.node.page.default:third_party_settings.layout_builder.sections.0.components.89467df8-8ca7-48a6-9f92-8292f125dd36.configuration.label_display failed with: variable type is string but applied schema class is Drupal\Core\TypedData\Plugin\DataType\BooleanData

Failed asserting that Array &0 (
    'core.entity_view_display.node.page.default:third_party_settings.layout_builder.sections.0.components.b961467b-96b6-47da-baab-fec95554f963.configuration.label_display' => 'variable type is string but applied schema class is Drupal\Core\TypedData\Plugin\DataType\BooleanData'
    'core.entity_view_display.node.page.default:third_party_settings.layout_builder.sections.0.components.89467df8-8ca7-48a6-9f92-8292f125dd36.configuration.label_display' => 'variable type is string but applied schema class is Drupal\Core\TypedData\Plugin\DataType\BooleanData'
) is true.
 

I've tried to debug where the config change is occurring that is inserting those two components but I've had absolutely no success. I'd like to get this issue over the line but this one has me stumped and I've hit a wall.

Would appreciate any help with this one!

Thank you

Surfaced again at #3519179: Cannot place a code component with an image prop, kudos to @longwave for connecting it to this issue!

I opened a new MR based on the old MR because both rebasing and merging ended up with way too many conflicts to resolve, I don't think this is complete by any means but it's a start.

The update path test is running into #3401876: Fix bugs in update path surfaced by config validation where block.block.claro_help_search does not have a UUID and so fails to save.

Pleased to see this one has got a bit of momentum again! I'm keen to see what I was missing :) Cheers @longwave!

This again came up in XB, this time it caused deterministic hashing to first seem broken/impossible, but @larowlan narrowed it down to this issue causing it.

To be more precise: XB has been using label_display: false in many tests, but due to this bug, that MUST be label_display: '0', otherwise deterministic hashing is impossible, because config schema always forces FALSE back to '0'.

We can work around it in XB by simply not using label_display: false 👍 See #3528159: Ensure deterministic version hashes for ComponentSource-specific settings, thanks to config schema-powered normalization.

Would be really great to get this fixed once and for all 😇

Hit this as well. As an extra cherry on top, what happened for me is - because the value is currently incorrectly maked as translatable - that on a multilingual all my block labels are suddenly "sichtbar" (which means visible in German).

Rebased and squashed. I "manually" diffed the change in core/modules/system/tests/fixtures/update/drupal-10.3.0.filled.standard.php.gz but could not find a meaningful difference, so just ended up reverting that, let's see if this passes or not.

So in fact the claro_help_search block was missing a UUID and this in turn triggered an error via \Drupal\Core\Config\Entity\ConfigEntityBase::preSave() when checking for existing entities with that UUID over in \Drupal\Core\Config\Entity\Query\Condition::compile() because that results in mb_strtolower() being called with NULL. Fun!

Let's see if this is better. The test passes now with a deprecation, but I think that's correct as update tests inherently trigger deprecations before the update... but we'll see.

So the "bare" fixture had the same issue of the missing UUID.

And as I hinted at in #69 I was wondering why not all of the update tests have #[IgnoreDeprecations] if the newly added one needs it and it turns out in fact that due to the added update hook, the deprecations are now triggered for all (?, at least many more) update tests, so fixed that now and everything is green. As far as I can tell, this is all how it should be because the deprecation is triggered in the pre-update code path, so there is no way to fix it (except to retroactively change the database fixtures, but that would be cheating, I think).

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

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

@tstoeckler Welcome to "fun with uuids!". See #3401876: Fix bugs in update path surfaced by config validation, specially #13 and #14.

Thanks for the pointer @penyaskito! Yeah, you seem to have hit the same problem there, just via a different path. Though it's not particular explicit this issue is (by accident) introducing the test that you are noting there in #14. As far as I can tell, though, it still makes sense to pursue both issues independently as they are tackling separate bugs, we just happened to have hit the same weird issue with the update tests.

Looking back I see that that issue and the block issue in already mentioned in #63. I must have read past that, sorry!

Rebased now and also added a deprecation for BlockPluginInterface::BLOCK_LABEL_VISIBLE per #62.

I only read over the changes in the MR, but I noticed a couple of things:

• The deprecation of BlockPluginInterface::BLOCK_LABEL_VISIBLE needs to be added to the proposed resolution section of the issue summary.
• I saw some instances where the text case of false was inconsistent in config files, mainly core/profiles/demo_umami/config/install/core.entity_view_display.node.page.full.yml and core/profiles/demo_umami/config/install/core.entity_view_display.node.recipe.full.yml. I'm uncertain how important it is. My guess is that there's no linter rule for this or it would have been triggered. But someone may want to double-check all of them.

Updated the issue summary, fair enough.

And nice catch on the casing in the YAML files. Fixed that now for the mentioned cases. It's true that it's not 100% percent consistent in core, but I agree that we shouldn't make it worse here.

This was suggested before #2908899: Wrong configuration schema for block_settings.label_display property and subsequently closed because it would be counter-productive for #2614950: Add option for visually-hidden block titles

I don't really have a strong feeling either way, so I would like to get the translatability problem that I mentioned above fixed without being stuck in a deadlock. I now found that that concrete problem was introduced in #3379725: Make Block config entities fully validatable so was not actually the case when this issue was opened. So I opened #3547808: label_display block configuration value should not be translatable so that we can fix that problem separately.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

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

Rebased (and squashed) and added the #[RunTestsInSeparateProcesses] to the update test added here per #3547849: Add #[RunTestsInSeparateProcesses] attribute to all Functional/FunctionalJavascript tests.

Appears to have merge conflicts now.

I resolved the merge conflicts, but the pipeline fails now. Presumably some more update tests that were added in the meantime need to have #[IgnoreDeprecations] added. And I guess more changes from #3547808: label_display block configuration value should not be translatable need to be reverted now (there were a couple of cases where boolean values were used which were changed to a string in #3547808 for consistency, that now need to be changed back with the MR here). Just in case someone wants to pick it up (because I don't think I will get to it anytime soon).

@tstoeckler: Don't we need to revert the constraints that #3547808 added? See https://git.drupalcode.org/project/drupal/-/merge_requests/11868/diffs#n...

Good point, thanks! Applied your suggestion.

Deprecations need to be for 11.4.0 now - we can probably get away with removing this constant in 12 though, given that it's meaningless after this change?

Alrighty, went ahead and rebased this and then fixed #81 and #85 now (and also updated the change notice accordingly). Needed a quick touchup for #3570668: Remove History module from core but it's green now.

I was a bit unsure about all the IgnoreDeprecation calls, but as explained in #70 that actually makes a lot of sense. Deprecations are now to 11.4.

I think this is a great improvement.

I'm not sure why I did not rtbc it in my previous comment, but I again looked through the mr and I don't find anything that should be changed.

Awesome, thanks @borisson_!

This needed a rebase due to the removal of migrate drupal (because we had to fix up some migration tests, which are now removed) and then some test fixes for the addition of the new admin theme (just the usual "'0' -> false" config changes like everywhere else) and the removal of ban and contact modules (just the addition of the "uninstall-{ban,contact}.php" scripts in the new update tests).

Leaving at RTBC as those are trivial changes and are inline with / identical to what was already RTBCed.

mini rebase, small issues, staying on rtbc

Merge conflict is still there.

Rebased, trivial conflicts due to #3581111: Balance Kernel and Functional tests.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

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

Discussed this in Athens with Alex Pott and @tstoeckler, we think that #2614950: Add option for visually-hidden block titles is a valid usecase and instead this issue should be closed. Instead of a boolean it should probably be an enum, which means that we should have a look at #2951046: Allow parsing and writing PHP class constants and enums in YAML files

Yeah, thanks for pushing for the clarification @borisson_! Setting the status to closed. This can always be re-opened if things change in the future, but for now let's be clear about the expectations of this landing.