Adding or editing a block through the UI saves the entity twice

Saving credit for @xjm for the review of MR 2905

Left some small changes on MR

Think a last step would be a CR for the new trait and it's usage.

CR draft created: https://www.drupal.org/node/3392235 →

Removing tag.

Taking a look at the CR and it reads very well. Listing the methods is perfect I think.

This is a surprising solution for a couple of reasons. We're adding a validateConfiguration() method to LazyPluginCollection and saying it needs to be called before saving but this is not really a validation. What should this method do if it gets invalid configuration? We're also adding a ConditionPluginCollection::getName() method that just seems odd. It's only there to provide a name for config schema to work. This change doesn't feel right but I've not really got an idea on how to fix this properly. The code in \Drupal\Core\Condition\ConditionPluginCollection::getConfiguration() that removes configuration if it matches the default doesn't really work the way the configuration system expects.

I guess I think the fix needs to happen on the form level as the incorrectly typed values are coming from there. This fix is currently implemented for all config entities in ConfigEntityBase::preSave().

There are two ways this makes things inconsistent:
1. Condition plugin config data is casted in ConfigEntityBase::preSave() rather than in \Drupal\Core\Config\Entity\ConfigEntityStorage::doSave()
2. Config plugins casting ignores the affects of trusting configuration data.

As said I'm not sure what the correct thing to do here is.

@alexpott
Updated the MR to do the casting at the form level (in submitConfigurationForm in ConditionPluginBase). Is this along this lines of what you were looking for?

Updated MR to address reviews and rebased for merge conflict.

Rewrite for Drupal 9.5.x.

Thank you @flitt1 but hiding #42, don't believe this will be backported and don't want it to be reviewed over the MR.

Believe feedback has been addressed. Only one i'm unsure is the point @xjm pointed out about the scheme change.

Discussed this issue with @bircher and both of us have concerns about leveraging config schema in the plugin system to cast a plugin's configuration. This is because there's no guarantee that the a plugin config is going to be saved in the configuration system.

We were both wondering whether or not we could change the comparison from === to == in \Drupal\Core\Condition\ConditionPluginCollection::getConfiguration() since we couldn't work out a situation where loose comparison would be a problem. Especially due to changes in PHP 8 type juggling.

alexpott → credited bircher → .

@alexpott
Updated MR and reverted everything that had to do with casting.

Also, made this change and added tests.

We were both wondering whether or not we could change the comparison from === to == in \Drupal\Core\Condition\ConditionPluginCollection::getConfiguration() since we couldn't work out a situation where loose comparison would be a problem. Especially due to changes in PHP 8 type juggling.

@godotislate the changes look beautiful. It's lovely that this has become much simpler and the comment you added to ConditionPluginManager::getConfiguration() is fantastic. Eagerly waiting for this to be RTBC :)

Adding contribution credit.

Went through the MR again and Thumbs upped threads I believe are resolved. Only 2 I didn't were the ones from @xjm.

Thanks for adding the comment if ($default_config == $instance_config) without reading the comment in #44 it was super clear why that decision was made.

LGTM

One piece of feedback. Also there are 17 other unresolved threads that *were* resolved. @godotislate as the MR author, you can mark those as resolved.

@alexpott pushed back on explicit test coverage for double-saving, but the last commit fixes things nicely enough

Committed and pushed 24b8523c4a7 to 11.x and 1ea22bd454b to 10.2.x and 78b55658d95 to 10.1.x. Thanks!

Automatically closed - issue fixed for 2 weeks with no activity.

godotislate → closed merge request !2905