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

This issue fixes the problem in #3114467: 'Negate' form value for condition plugins should be cast to boolean in validation. in a lower level.

@tim.plunkett Sorry, missed your last comment from months ago.

I retried with both Drupal 9.4.8 and 10.1.x-dev with Moderation Dashboard 2.1.1, and I don't see the error anymore. All other functionality seems to work correctly.

Setting back to Needs Review to kick off CI.

ETA. Looks like there'll need to be a new MR against 10.1.

Created new MR against 10.1 and tests passed. I'd say RTBC, but I guess it's technically my MR now.

https://git.drupalcode.org/project/drupal/-/merge_requests/1456.patch I tried using this patch for 9.4.7 (and 9.4.8), but patch does not complete.

Attaching a screenshot.

Maybe I am misunderstanding how to use the new way to get patches? https://www.drupal.org/docs/develop/git/using-git-to-contribute-to-drupa...

Testing https://git.drupalcode.org/project/drupal/-/merge_requests/2905

Checked that the tests fail without the fix and passes with the full MR.

Didn't see any merge conflicts (didn't rebase)

Left a few nitpicks and some questions on the MR. Thanks!

@smustgrave, how did you check that the test fails without the fix? If you did so locally, I'd paste your shell output to receive credit for that testing.

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

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