Field [storage] config have incomplete settings until they are saved

Running into this over at #3324140-22: Convert field_storage_config and field_config's form validation logic to validation constraints → .

And TIL I was pinged to review/provide feedback on https://git.drupalcode.org/project/drupal/-/merge_requests/3926#note_189783 2 days ago, and that happens to run into exactly this problem space, but not \Drupal\field\Entity\FieldConfig::preSave() specifically: field_field_config_presave() is doing things that are critical for certain FieldConfig entities (those that are for Entity Reference fields) to be correct/saveable.

Increasing priority.

Committed a version of the patch that doesn't lead into WSOD to see how many test fails it causes. I expect there will be a lot of test fails because normalizing the config in preSave could be the expected behavior.

Hiding patches in favor of the merge request.

I watched with increasing bafflement as @lauriii and @catch got into the weeds discussing this issue's backwards compatibility concerns on Slack. After that, I finally Zoomed with @lauriii to get the executive summary and understand it for myself.

The plan is this: we want the calling code to be responsible for merging the default settings into FieldConfigStorage::$settings (and its counterparts in BaseFieldOverride and FieldConfig). postCreate() will do this for you when it creates a new entity (by calling the $this->initSettings() method). In other situations, you need to do it yourself, and preSave() will check your homework -- $this->settings must contain every key in the default settings, or you'll get a deprecation notice (and maybe, in Drupal 11, an exception). postLoad()'s behavior will be unchanged.

Left a couple of comments on the MR, the remaining fails are all because of the new deprecation. Do we solve those in this issue as well?

Assigning to myself to fix the tests and find out which code paths are triggering these deprecations.

Did a review. issue summary definitely needs an update.

What I don't quit understand is why we plan to enforce that settings are passed in, we have the default settings, we can complete them, why not keeping doing that? That's how plugins work as well (19 instances of $this->configuration = $configuration + $this->defaultConfiguration(); in core, I think a ConfigurablePluginBase would be neat ;))

IMHO, this is going to hurt us down the line. This would make adding a new setting to a field type plugin a BC break as all existing code who creates fields of that type is going to break.

Less sure about invalid settings, I can see how that's more sensible to enforce, but we would have the same problem then if we remove a setting from a field type, then that's suddenly invalid too.

#43 mentioned an issue summary update.

And number of test failures in MR.

Updating the issue summary with the proposed resolution. I'm also tagging this for a change record, which I'm pretty sure will be needed.

There's one more soft blocker to 📌 Save FieldStorageConfig at the same time as FieldConfig Fixed related to invalid config which should be resolved either by this issue or a follow-up. The field_field_config_presave normalizes the selection handle plugin IDs and we need to manually duplicate that so that we can run it earlier. Thoughts on whether we should address that here or in a follow-up issue?

Okay, filled in the CR!

Re #48, I think we should do this in a follow-up. As you can see, I did a little experimentation around this but it seems rather tricky, and I'm not sure that experimental approach was the right one anyway.

First: AWESOME progress here! 🤩

Thanks for #39, that's really crucial context wrt BC concerns and release manager guidance! Linking it from the issue summary 👍 Makes sense that we don't want to modify postLoad() at all in particular, because that would have the biggest BC (runtime) impact.

I have to agree with @Berdir in #43: that simple approach seems preferable and less disruptive. But AFAICT since then, that was largely adopted. In reviewing this, I did spot a few more BC concerns though. 😅 Or at least, potential ones — hopefully you can show why none of them are actual BC concerns! 😄

Regarding @lauriii's remark in #48: I think that if we're making the behavior of Field(Storage)Config's creation/saving logic sane & centralized, that it makes sense to consider field_field_config_presave() in scope too — it's touching the same problem space.

Reassigning to Wim for another round of review.

This feels much closer now thanks to your work on FieldConfigBase::set('settings', …)! It allows us to revert a LOT of changes.

I see that you commented many times that that was impossible, but … I think you wrote those comments before you did that work? 😅 In my spot testing, it sure does work every single time!

Also figured out why there was a weird deprecation-testing-related test failure. The root cause is simple: thanks to the excellent work you did on FieldConfigBase::set('settings', …), the deprecation error

Saving a field_config with incomplete settings is deprecated in drupal:10.2.0 and will throw an exception in drupal:11.0.0. Adjust your code to ensure that all supported settings are provided. See https://drupal.org/node/3375334.

simply cannot ever occur anymore! 😅

Before this can be RTBC'd, this definitely needs an issue summary update.

Also, thanks to FieldConfigBase::set('settings', …), I think the CR needs to be updated? 🤓

Do we still need a follow-up here?

I ask because...as far as I can tell, it's no longer possible to put incomplete settings on a field or field storage config entity, unless you use reflection. So rather than deprecate, we can just throw an exception.

Looking great!

But in doing a thorough final review, I made one very startling discovery:
\Drupal\Core\Field\FieldItemInterface::fieldSettingsToConfigData() has existed since 2014 and AFAICT does exactly the same thing as the new \Drupal\Core\Field\FieldItemInterface::normalizeSettings() we're adding?! 😳

I see a way forward thanks to https://www.drupal.org/node/3376455 → ! 🥳

Also still needs a change record for the migration change.

Those are the only 2 remaining threads 👍

This is ready, but still needs change record updates, as well as 2 additional change records (the new interface argument and the modified migration destination). It also still needs a follow-up for the @todo that was just added for Drupal 11.

Once those things are done … this will be RTBC! 😄🥳

Okay, the change records have been added/updated.

Opened 📌 [Drupal 11] Require $field_definition to be passed to FieldItemInterface::fieldSettingsToConfigData() Closed: outdated to change the interface method in Drupal 11, and linked to it in the todos.

I have read trough the PR several times, in the hope to find something that was not clear, could use better documentation or can otherwise be improved. I can't seem to find anything.
I also read through the Change Records, those are very clear as well.
I think that means this can go to rtbc, but I would feel more comfortable with one of the field or entity api maintainers +1 here as well. Do we need to tag this as needs subsystem maintainer review?

Agreed with @borisson_; tagged 😊

Refined the migration destination CR → , added link to 📌 [Drupal 11] Require $field_definition to be passed to FieldItemInterface::fieldSettingsToConfigData() Closed: outdated to the fieldSettingsToConfigData() CR → .

The third CR (for field config/storage settings) → sounds … a bit like it's breaking quite a lot of things? 😅🙈

So let's see if all those *Test.php changes are necessary, by reverting all modified tests. It's easy to get those by comparing to the last upstream commit that was merged in:

git checkout 2327883-field-storage-config
git pull
git diff e72fa7b3759972013c3e943372baaae85722fbfe HEAD -- **/**Test.php

Because if we could show that none of those test changes are necessary, then we simultaneously prove that the risk of disruption/BC break is extremely low. And … unless I made a mistake, every single one of the tests still passes if I revert all test changes! 🤩 The changes were definitely necessary at an earlier iteration of this merge request, but now they're no longer necessary: thanks to core/lib/Drupal/Core/Field/Entity/CompleteFieldSettingsTrait.php, and the updates to core/lib/Drupal/Core/Field/FieldConfigBase.php and subclasses, everything is guaranteed to be complete, always! 😊

The sole exception: \Drupal\KernelTests\Core\Field\FieldSettingsTest. But that makes sense: it's literally testing the updated behavior in a unit-like kernel test that checks how Field(Storage)Config entities are created and how their settings are saved 👍

**Test.php didn't catch everything. Reverting the changes in FileFieldTestBase.php and ImageFieldCreationTrait appears to work fine too 🤓

Now, to be fair, the code that I reverted in the last 2 commits was:

1. actually more accurate, even though unnecessary
2. going to be reused in its entirety in 📌 [PP-1] Convert field_storage_config and field_config's form validation logic to validation constraints Postponed or one of its child issues, because then those invalid settings should start triggering validation errors!

But AFAICT this means that the third CR ( https://www.drupal.org/node/3375334 → ) can simply be deleted — or at least associated with 📌 [PP-1] Convert field_storage_config and field_config's form validation logic to validation constraints Postponed instead of this issue! 😊 IOW: none of the work was in vain, but let's keep this issue's scope as tight as possible and hence make it easier to get this issue committed 🤞

This also should make subsystem maintainer review a lot simpler.

Still green. This is IMHO RTBC now.

I would like explicit confirmation for the 2 commits I pushed from @phenaproxima and @borisson_. After that, we need subsystem maintainer review.

I have no objections to Wim's changes. Fewer files touched is good, and indeed, we can delete the third change record -- even nicer!

Fewer touched files is indeed more better.

Associated the 3rd CR with #3324140 instead 👍 2 CRs left here.

Posted a couple of comments in the MR.

I think a possible solution to the first problem (calling fieldSettingsToConfigData() in FieldConfigBase::postCreate()) is to move the code which sets the handler setting for ER fields from field_field_config_presave to a hook_entity_create() implementation.

Thanks, @amateescu! 😊🙏

Assigning to @phenaproxima to address your remarks, but … I did ask 2 clarifying follow-up questions on the MR, and I have one other question for you here: 🤓

[…] move the code which sets the handler setting for ER fields from field_field_config_presave to a hook_entity_create() implementation.

It sounds like this means that my discovery/"aha" moment in #59 was wrong? 😅 @phenaproxima was actually first adding a new FieldItemInterface::normalizeSettings() method, and then in doing a final review, I discovered ::fieldSettingsToConfigData(), which I'd never heard of before, and it seemed to be doing the exact same thing.

@Wim Leers, unfortunately.. yes :) I learned about fieldSettingsToConfigData() from this issue, but then I read the issue where it was introduced and realized that it should only be used prior to saving a field to config.

As for a test case about not calling fieldSettingsToConfigData() in postCreate(), a very good example is \Drupal\options\Plugin\Field\FieldType\ListItemBase::storageSettingsToConfigData() and \Drupal\options\Plugin\Field\FieldType\ListItemBase::storageSettingsFromConfigData(), which show how/why a field type might need to have a certain structure for its settings in the runtime object, and a different settings structure when stored in config.

Okay, great, thank you SO MUCH for confirming that!

Sounds like we should restore the ::normalizeSettings() approach! 😇

This is ready now, although an explicit confirmation from @amateescu sure would be nice! 😇

Sounds like we should restore the ::normalizeSettings() approach!

Is there a problem with the hook_entity_create() suggestion for setting the handler setting of ER fields? A hook implementation has the advantage that it can be removed at any time if we refactor the code around selection plugins and their special handling of plugin IDs, while a new method on FieldItemInterface would take a major release cycles to deprecate and remove.

Is there a problem with the hook_entity_create() suggestion for setting the handler setting of ER fields? A hook implementation has the advantage […]

Yes, there is a significant problem with that: it gets us the same exact problem that field_field_config_presave() causes today: modules like https://github.com/jibran/dynamic_entity_reference then have to hack their way around it with hook_module_implements_alter() because these hooks cause the field's (entity reference selection handler) settings processing to happen at the wrong level!

See #33, which points to @larowlan's comments, and most critically, to https://git.drupalcode.org/project/drupal/-/merge_requests/3926#note_194955. See https://git.drupalcode.org/project/dynamic_entity_reference/-/blob/3.1.0....

Basically: while we're touching this code, we might as well make it so that core won't be breaking contrib modules in this way anymore.

it gets us the same exact problem that field_field_config_presave() causes today: modules like https://github.com/jibran/dynamic_entity_reference then have to hack their way around it

That's very easy to fix though: we can just check if the field type provides a target_type storage setting and return early if it doesn't.

#80: Aha, no, because target_type is the only additional key that \Drupal\Core\Field\Plugin\Field\FieldType\EntityReferenceItem::defaultStorageSettings() specifies! So yes, then this could totally work, and we avoid increasing the API surface.

I think the solution that we had arrived at here is nice & fine. It seems that @larowlan, @jibran, @phenaproxima and I were all fine with it (maybe @lauriii too, not sure). But it does mean expanding the Field API more, and if we can avoid that, that'd be better. It'll also mean a simpler upgrade path from Drupal 10 to 11, which in its own is worth a lot (i.e. no field type implementation changes necessary!).

So: +1 to restoring field_field_config_presave() and returning early if target_type is absent from the default storage settings. Thank you for helping keep scope & impact as minimal as possible, @amateescu! 😊🙏🙏🙏

After these twists and turns, the IS became outdated again, and we're missing a change record for the solution that @amateescu helped us arrive at. Tagging for both.

Thanks for the updates, this looks much cleaner now. Left two more comments on the MR then this can be set to RTBC.

@Wim Leers, if you mean that we need a change record for the field_field_config_presave() update, we can just tell @larowlan and @jibran about it and save a few hundred people from reading stuff they'll never have to care about :)

This CR → needs to be deleted though.

OK, I've made the requested changes here. Re-assigning to Wim for a final review, now that we have subsystem maintainer approval, and for change record/issue summary updates.

If I read #72 and #78 correctly, it's proposing moving the entity reference normalization from hook_field_config_presave to hook_field_config_create which should be sufficient to ensure that entity reference fields are valid after creation.

However, the MR currently keeps the normalization in hook_field_config_presave which is insufficient because it means that the handler is normalized only at the saving stage. It also looks like tests are passing which means that we have insufficient test coverage.

I confirmed that the MR as it is is not sufficient by confirming that \Drupal\Tests\field\Functional\EntityReference\EntityReferenceAdminTestfails with the current MR and a version of 📌 Save FieldStorageConfig at the same time as FieldConfig Fixed that doesn't have the hard coded normalization as part of field creation.

Discussed with @phenaproxima on Slack because there were some confusion between comments #72 and #81. As I mentioned in #86, I understood #72 and #78 so that @amateescu is proposing to move the entity reference normalization from hook_field_config_presave to hook_field_config_create. I said in #86 that it should be sufficient to ensure that entity reference fields are valid after creation.

However, @phenaproxima pointed out that it won't be sufficient because the handler settings may change during the life cycle of the field config. This is why we would need at least hook_field_config_presave and hook_field_config_create to normalize the config. However, this defeats the whole purpose of this issue because the intent is also to be able to validate the config before saving it. For this reason, we would need the normalization to happen on a separate layer, that could be triggered before saving.

I'm wondering if this provides sufficient justification for the new API that we were proposing to add until #72 😊

Discussed with @amateescu on Slack and he said that even with the justification in #87, he would prefer to introduce the new API as part of 📌 Combine field storage and field instance forms Fixed . I can see how that makes sense because then we can make sure that the API is flexible enough to support the needs of that issue.

Doing the entity reference normalization both, on hook_field_config_create and hook_field_config_presave addresses the bug where field config entities are invalid after creation which blocks both 📌 [PP-1] Convert field_storage_config and field_config's form validation logic to validation constraints Postponed and 📌 Save FieldStorageConfig at the same time as FieldConfig Fixed . We can't remove hook_field_config_presave without introducing the new normalization API because existing code may be relying on it, e.g. field_field_storage_config_update in core.

I think it would be great if we could add test coverage that I asked in #86 to make sure that this doesn't regress until we have the additional test coverage from 📌 Save FieldStorageConfig at the same time as FieldConfig Fixed .

Does that mean DER can remove the hook implementation alter and implement it's own hook_field_config_create and update existing hook_field_config_presave?

@jibran yes, the current implementation would allow you to remove hook_module_implements_alter from DER once 10.1.x is EOL 😊

• #86 + #87: nice deciphering of this very complex situation! 🕵️‍♀️
• @amateescu's commit (https://git.drupalcode.org/project/drupal/-/merge_requests/4371/diffs?co...) looks great 👍

This is ready, just needs a change record. Since this is blocking high-impact usability issues, and the CR would affect so few people, I think it'd be fine to land this ahead of the CR existing 😇

I'm not sure if we'll get away with not having a CR but it would be nice to have an updated IS 😇

@phenaproxima will tackle the issue summary update + change record tomorrow.

But more importantly, @Berdir has surfaced concerns with the current approach, which was proposed by his fellow subsystem maintainer @amateescu 😄 So, I think we really need @amateescu's input now.
(I did point @Berdir to the relevant comments though, so maybe that is already enough? 😇)

We discussed my feedback in slack and I'm fine with addressing that in a follow-up.

In short, the situation for DER is indeed improved as I realized in my second comment due to the target_type check, I still think this should target specific field types only or at least give subclasses a way to customize it, but it is in line with the existing logic and behavior and that's exactly what the follow-up is about.

Unassigning since @amateescu was part of the discussion mentioned in #94 and it sounded like everyone was onboard with the current solution.

Still needs an issue summary update and change record 😇

Partial IS update.

IS looks great! https://www.drupal.org/node/3379013 → is still relevant.

Change record written and the issue summary is rounded out. https://www.drupal.org/node/3381705 →

Thanks for https://www.drupal.org/node/3381705 → !

Let's do this :D 🚢

🧐 Let's see if the deprecation @lauriii and I thought would make sense triggers failures in core's test suite. Note that for now, I did not yet do the conditional part. If there are failures specifically for FieldStorageConfig settings being set on FieldConfig, the commit I just pushed should fail tests.

Moving to needs review because there has been changes to the MR that would benefit from further review.

Discussed with @Wim Leers, @phenaproxima, @tedbow, and @tim.plunkett.

First of all, there are 2 hard requirements:

1. We absolutely cannot break BC (meaning ANY change in behavior) at any point in the life cycle of Field(Storage)Config
2. We absolutely need the ability to have complete Field(Storage)Config before saving

We cannot normalize in ::setSettings() because it's possible for any contrib module to call ::set('settings', …) instead. (Plus, \Drupal\Core\Entity\EntityForm::copyFormValuesToEntity() also calls ::set(…)!)

👆 Hence the current approach in the MR: to normalize during both ::set() and ::setSettings().

⚠️ However, we discovered that we cannot do it in ::set(), ⚠️ because it could introduce very complex issues to debug. The key challenge is that ::set() is being called

• At SAVE time by \Drupal\Core\Config\Entity\ConfigEntityStorage::doSave after it has called \Drupal\Core\Config\Entity\ConfigEntityStorage::mapToStorageRecord and \Drupal\Core\Config\Entity\ConfigEntityStorage::mapFromStorageRecords
• But it's not called at LOAD time by \Drupal\Core\Config\Entity\ConfigEntityStorage::doLoadMultiple

… meaning that normalizing in ::set() does lead to differences in settings depending on where in the life cycle the config entity is, i.e. after saving the entity, it would be in the normalized state, but after loading it from the database, it would NOT be in the normalized state. This is highly confusing, and breaking BC for \Drupal\Core\Config\Entity\ConfigEntityStorage::mapFromStorageRecords during save.

This leaves only one option that we can see: restore FieldItemInterface::normalizeSettings(), make it a public method, so that it can be called e.g.

A tangentially related problem is that the sole thing in Drupal core testing this is \Drupal\field_test\Plugin\Field\FieldType\TestItem::storageSettingsToConfigData() + \Drupal\KernelTests\Core\Field\FieldSettingsTest::testConfigurableFieldSettings(), but TestItem methods are adding NEW keys instead of transforming the values of EXISTING keys. The latter was always the intent — see #2293773: Field allowed values use dots in key names - not allowed in config → . So we should fix the test coverage around TestItem::class because it's currently asserting things that do not make sense.
In the absurd case that something in contrib is relying on this: we can and should detect incorrect implementations of toConfigData() and fromConfigData(): any key-value pairs that it returns MUST also exist in \Drupal\field_test\Plugin\Field\FieldType\TestItem::defaultStorageSettings(). If that's not the case, that should trigger a \LogicException or at least a deprecation.

See [#2307365]!

Discussed with @amateescu on Slack: https://drupal.slack.com/archives/C1BMUQ9U6/p1692622430919759.

The key insight is that @amateescu pointed out that we provide an API that allows runtime field [storage] config entities to NOT be valid config. The API that allows this is \Drupal\Core\Field\FieldItemInterface::storageSettingsToConfigData and \Drupal\Core\Field\FieldItemInterface::fieldSettingsToConfigData, because these are required to have run for some field types before they can be valid config entities to store. Given that there is a difference between the stored config entity, and the runtime entity, he also pointed out that he doesn't see why the solution in #2293773: Field allowed values use dots in key names - not allowed in config → should be limited to massaging existing values.

Given this, to be able to validate field [storage] config, we'll need to trigger \Drupal\Core\Field\FieldItemInterface::storageSettingsToConfigData and \Drupal\Core\Field\FieldItemInterface::fieldSettingsToConfigData before triggering config validation. This could be done in an ephemeral object used for validating the config.

The proposed changes to the current MR:

• Merge default settings only in postCreate
• Stop doing it on set, setSetting, setSettings to avoid introducing changes with the runtime field config object
• Move the current trigger_error code from \Drupal\Core\Field\FieldConfigBase::mergeDefaultSettings to \Drupal\Core\Field\FieldConfigStorageBase::mapToStorageRecord. This should be run after \Drupal\Core\Field\FieldItemInterface::storageSettingsToConfigData and \Drupal\Core\Field\FieldItemInterface::fieldSettingsToConfigData to make the callbacks responsible for normalizing the settings before saving.

Implemented #104 with the exception that I left the preSave changes to a follow-up. There's around 50 tests failing if we introduce the deprecation. I think we may want implement the changes to preSave, but that's not needed for either of the blocked issues. I think we could open a follow-up for that.

I'm wondering if we could now revert the migration related changes too and move them to a follow-up? Maybe @phenaproxima could confirm that.

On top of that, we need to update CRs and the issue summary.

Updated the issue summary and the change records.

Not going to file an explicit follow-up: we can define what we need in 📌 [PP-1] Convert field_storage_config and field_config's form validation logic to validation constraints Postponed .

Thanks for the updates, @lauriii, let's move forward! :)

I dug through this in detail with @laurii and I'm confident that we can make this work for config validation support in Field(Storage)Config + BaseFieldOverride.

One thing I definitely hadn't thought of yet so far is how config entity objects may have different values in their runtime state vs their storage state, thanks to \Drupal\Core\Config\Entity\ConfigEntityStorage::mapToStorageRecord()! 😳 Fortunately, only 3 classes in core override it:

1. \Drupal\Core\Field\FieldConfigStorageBase::mapToStorageRecord()
2. \Drupal\field\FieldStorageConfigStorage::mapToStorageRecord()
3. \Drupal\layout_builder\Entity\LayoutBuilderEntityViewDisplayStorage::mapToStorageRecord()

… plus 6 in contrib, 5 of which are virtually identical to core's Layout Builder one.

This means that for config entity validation to work, the infrastructure that provides validation for config entities must also be able to call mapToStorageRecord() not just prior to save, but prior to validation. But that method is currently protected 😅 So we need some way to get config entities into the same state as just before saving. We may want to do that as part of 📌 Split validation-related methods from FieldableEntityInterface into a separate ValidatableInterface Needs work or in another issue.

In any case, it's clear that it's possible, that this change won't block it. That's the thing we wanted to make sure.

This has been very thoroughly reviewed, and the end result after >100 comments is a far smaller patch with a tighter scope than before subsystem maintainers started reviewing it. Special thanks to @amateescu & @Berdir! 😊

EDIT: cross-post RTBC 😅🥳

Overall, I think this patch makes a lot of sense. I'm glad it ended up with a fairly simple implementation. Since there might be some subtle BC breaks here, before committing it I want to read through the comment history to see what's been discussed as far as that goes. In the meantime, these are the only two things that jumped out at me:

1. From the IS:
   

   If settings are changed any point (either by setSettings(), setSetting(), or set('settings', ...)), the default settings will be merged in automatically

   Where is this done?

2. +    // Filter out any unknown (unsupported) settings.
   +    $supported_settings = array_intersect_key($this->getSettings(), $default_settings);
   +    $this->set('settings', $supported_settings + $default_settings);
   

   We need a change record for this (either added to https://www.drupal.org/node/3381705 → , or in my opinion it would be better as a separate CR).

Thanks for the review @effulgentsia! Looks like we had an outdated paragraph in the issue summary. We're not doing that anymore due to the DX concerns raised in #103.

Updated the CR. I think it's fine to have one CR since copying the default values should matter less.

Something that's worth noting is that the deleting of unsupported keys is currently done in \Drupal\field\Entity\FieldConfig::preSave so adding that step to the postCreate() might not be as big of change as it seems.

I read through this issue's comments, and I didn't see anything in them that concerned me about committing this as an interim improvement. The issue summary mentions that additional changes/APIs are likely needed, but those can happen in their respective issues.

Therefore, pushed this to 11.x! Great work, everyone!

lauriii → closed merge request !4371

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

It is more and more likely that the commited change breaks site install from existing config when Content Moderation is in use...

More details in #3441962-15: Call to a member function getSettings() on null in Drupal\Core\Field\FieldConfigBase->getSettings() →