Save FieldStorageConfig at the same time as FieldConfig

Since this broke so many tests that had to be fixed seems like we could be introducing a breaking change for some modules. Think a change record could be required.

A lot of the test changes look super weird at first glance but looking closer, they mostly seem to have relied on quirks of half-saved fields and so on, so that seems quite OK.

Left one comment about a dependency between field and field_ui modules.

Didn't review the field UI changes yet properly, that seems quite complex. I suppose the tempstore business is probably temporary until the other issue then fully merges those forms together?

Posted an initial review.

This issue would really benefit from an issue summary that summarizes what the architectural changes are, and why those make sense. That'd greatly simplify and accelerate the review process!

Describing the API changes in the issue summary 👍

From what I can tell the first and last thread still need to be addressed.

I went through the comments and looks like most of them are addressed. I think we still need to address the feedback regarding the redirects: https://git.drupalcode.org/project/drupal/-/merge_requests/3926#note_176826

Addressed feedback related to redirect.

Appear to be 2 open threads still.

Did do some light testing.
Added a field and made it all the way to the last step before clicking out of the field save page.
Added a field again with same machine name and the name was not taken so it allowed me to continue.
Was still able to create fields without issue.

For the change record.

Open threads and CC issue

This could use test coverage that checks the tempstores are updated as expected after various UI interactions.

Did the same test as before

Added a field and made it all the way to the last step before clicking out of the field save page.
Added a field again with same machine name and the name was not taken so it allowed me to continue.
Was still able to create fields without issue.

Still convince this will need a change record though. Can see a lot of modules are going to have to update their tests now

I agree with @smustgrave that this needs a change record. 👍

I've read through this MR and tbh I really can't see how it helps #3347291: Combine field storage and field instance forms. All these little hacks that are added all over the place seem to just create more work for the followup, since that one is supposed to clean up most of the changes done here.

Discussed with @amateescu on Slack to explain why we want to do the extra work to separate this work from #3347291: Combine field storage and field instance forms. It seems that the plan itself makes sense after a bit of explanation.

The main remaining concern is that we want to make sure that we have a solid plan to get rid of the hacks introduced here. To make reviewing this and #3356894: Make field selection less overwhelming by introducing groups easier, it would be really helpful if we could add @todo remove in #3356894 comments to places we definitely want to consider removing there. This way as we review this issue, we know which parts we expect to stay and which parts are expected to be removed. This also helps us check that all of the places we intended to remove, were indeed removed.

Started my review.

I think we can use type declaration for all class properties so please update those. I commented on a bunch.

Still getting my head around this.

@narendraR. I am still looking at this MR but I think there is some stuff that can already be addressed in my review. I will look at it more tomorrow also.

thanks

Per https://git.drupalcode.org/project/drupal/-/merge_requests/3926#note_189783, AFAICT this is blocked on #2327883 — see #2327883-33: Field [storage] config have incomplete settings until they are saved.

Left a review, and Wim has too. Updating status, although 'Active' would also be appropriate

This issue is essentially blocked by #2327883: Field [storage] config have incomplete settings until they are saved as discussed with @larowlan and @Wim Leers in the MR. The tests continue to pass because we are hardcoding the field config related normalization in Field UI, but to keep the solution generic enough for contrib, the blocker needs to be resolved before we can land this. Let's continue working on the MR to make sure that every other aspect of this is ready to go once the blocker has been resolved. 😊

We should update the issue summary with the changes to the approach.

This is looking far cleaner already! 🤩

I think there's only one hard blocker left: the Dynamic Entity Reference concern that @larowlan raised. That's impossible/does not make sense to solve here, it must be solved in #2327883: Field [storage] config have incomplete settings until they are saved. Three of the 12 remaining comment threads on the MR are about this! For that reason I'm tempted to prefix the title with [PP-1] and change the status to Postponed.
EDIT: hah, @lauriii already did that in #34! 👍

However, let's get this issue as ready as possible before doing that: let's get this "pre-RTBC", with the only needed remaining change being the removal of ~20 LoC from FieldConfigAddController::fieldConfigAddConfigureForm() once #2327883: Field [storage] config have incomplete settings until they are saved lands, that would otherwise break https://www.drupal.org/project/dynamic_entity_reference!

2 of the 12 remaining comment threads can be marked resolved — GitLab does not allow me to do that 🤷‍♂️

That leaves only 7 comment threads! 🚀

Looking good. Just notice a few more things in the MR

I think this is pretty much ready at this point!

While we're waiting for #2327883: Field [storage] config have incomplete settings until they are saved to land first, let's at least already update the issue summary here while it's still fresh in mind — hopefully that issue will land soon, but that's impossible to predict.

Adjusting the issue summary because many of the underlying issues we were solving here earlier have already been committed in #3345149: Extra Default value field when adding a field with an unlimited values and #2327883: Field [storage] config have incomplete settings until they are saved.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch 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.

@tim.plunkett AFAICT all remaining @todos now point to the follow-up, not the blocker? 😃 So… nothing left to do on that front after @lauriii's small removal-only commit 👍

Let's get that issue summary updated and change record created so that this can move to Needs review after ~1 month of being blocked!

Cross-posted with @lauriii 😆

I think we can make the change record more explicit by telling contrib developers that if the string Save field settings appears anywhere in their codebase, that they are probably affected.

This looks very close — only nits 👍

Thanks, https://git.drupalcode.org/project/drupal/-/merge_requests/4667/diffs passing tests separately means this is purely additional test coverage 👍

Just one question remaining on the MR :) Once that's addressed and a change record exists, I'll RTBC 🚀

Added the isNew check to FieldStorageConfig::hasData() and created a CR for it.

I closed the new MR to reduce chances of confusion.

I updated the CR to explain the changes on a higher level, in case there's some custom code that needs to be updated.

Looks ready to me! 😄🚢

Ticking credit boxes for all the reviewers.

I think overall this MR is looking great and I'm pretty close to wanting to commit it. @lauriii, @tim.plunkett, and I also polished up the change records.

When reviewing this, I started wondering about whether the use of a private temp store would introduce problems if two people were trying to add the same field at the same time, and then I saw that @larowlan also asked that same question in an earlier comment:

What happens in the scenario where admin A is creating field 'field_title' on a node type and in a separate session admin B is also creating 'field_title'?

Because we're using private tempstore there is the chance that their edits/changes could conflict/collide.

@Wim Leers responded with:

I'm not too concerned about private tempstore and collisions though, because that'd cause a collision at the final "save" time.

I think it would be good to add a test for what the expected behavior is. For example, is the expected behavior that whoever completes the final step first gets their work saved, and then the second person gets an error message when they try to save? Or does the second person overwrite the work of the first person without knowing about that prior work? Or something else?

We have test coverage for edge cases related to temp store in \Drupal\Tests\field_ui\Functional\ManageFieldsTest::testAddFieldWithMultipleUsers and \Drupal\Tests\field_ui\Functional\ManageFieldsTest::testEditFieldWithLeftOverFieldInTempStore 😊

The MR now has a merge conflict due to #3382802: Disable the 'Remove' button if there is a single row in the allowed values storage setting of the list fields.

Here's a patch that rerolls for that conflict. There's no substantive difference, only context lines differ.

Pushed to 11.x and published the CRs. Great work, everyone!