Combine field storage and field instance forms

Here's my current work in progress.

42 of these failures are specifically looking for "Save field settings" which is no longer one of the steps of the flow.

I think the breadcrumb thing was a red herring.
Will discuss with @lauriii and @bnjmnm

The entity reference fails are tricky. From what I can tell, the previous two-step process let you set the target type in one step and the target bundles in the second step. This is now one step, but the bundles need to be dynamically updated based on the target. I guess this means we need some JS? And if the solution is JS, we need a workaround for the Functional tests (or move those to be FunctionalJavascript)

Pushing changes. Tried to use provide a button using js-hide. Found a FormAPI bug which will eventually need to be split out, but the tl;dr is:

• in order for a partial-submit button to use #limit_validation_errors
• using #limit_validation_errors needs knowledge of #parents
• a button in a subform needs won't know it's #parents until a #process callback
• changes to buttons in #process callbacks happen *AFTER* the button is stored as the triggering element

This commit attempts to work around that

This is essentially blocked by #3358049: Save FieldStorageConfig at the same time as FieldConfig since making this change without it would be really hard.

This is a WIP patch against Drupal 10.1.x that combines the field storage and field instance forms on top of the MR in the blocking issue #3358049: Save FieldStorageConfig at the same time as FieldConfig.

#2327883: Field [storage] config have incomplete settings until they are saved has landed. Still blocked on #3358049: Save FieldStorageConfig at the same time as FieldConfig.

#3358049: Save FieldStorageConfig at the same time as FieldConfig has landed 🎉

Yay for the green test run! 🎉

Next step is to clean up the code and get some code reviews 👏

Added a failing test demonstrating the issue with the default values widget in case of list type fields.

Created a CR.

Believe all threads have been addressed.

Tested this manually by applying MR 4679
Created a new field and verified the storage and settings were on the same
Saved the field without issues
Edited the field as normal.
Verified existing fields before I applied the MR are also showing both storage and settings and working as before.

@lauriii and I discussed this issue today - I've left some review comments on the MR as a result that need to be addressed.

We also spent time discussing how and if code like https://git.drupalcode.org/project/date_occur/-/blob/0.1.x/date_occur/sr... would be affected and we tried a few things out and it looked like this wouldn't actually need any changes - which is fantastic.

Also we spent time discussing field_field_storage_config_update() and realised that this will result in multiple field config saves if you change an entity reference field's target type. Given the way a field storage can affect multiple fields there really is not any work around - so we felt it is best to leave things as they are.

Addressed all the feedback.

@srishtiiee the changes to the options module look great. Less alters FTW!

Looks like all of the feedback has been addressed and the most recent comment looks good! Moving back to RTBC.

(crosspost with @alexpott *and* @laurii, not changing the status for now)

I'll try to have a look at this, also with some of my contrib projects that to slight alterations to those forms, like TMGMT and Paragraphs.

One thing I'm surprised to not see here is a change to comment_form_field_ui_field_storage_add_form_alter(), is it possible we have no test coverage for that? That won't work anymore, correct?

#29 said that "a" field was tested, but I'm pretty sure this should be tested with all available core field types, specifically more complex ones like entity reference, file and image. The screenshot only covers one example. file and image specifically are known to have a very confusing split between field and storage settings and settings that can no longer be altered once data exists even though there's no reason for that. I wonder how that looks and is affected by this.

Instead of just wondering, I did play around a bit with image and file fields, and it seems mostly ok and I guess not worse than before, although the file/image add forms are *quite* long and messy, but at least you can see all available settings on one screen now.

One small issue I found is that #states of the Files displayed by default field in FileItem no longer works, there might be other uses of #states that are broken in core and contrib due to this.

I first thought that the $has_data logic is broken, because the uri scheme setting on image was not disabled when there was data, but actually, ImageItem for some reason redefines that setting from the parent FileItem implementation and doesn't include the #disabled definition? I think there's an existing issue to remove that from FileItem as well, that just makes no sense, especially when image doesn't follow that.

Isn't comment_form_field_ui_field_storage_add_form_alter() altering the first step of the field creation and therefore not related to this change? However, for the hook_form_field_storage_config_form_edit_alter use case we could potentially provide BC layer. Here's a POC for how that would look like.

We have been doing fairly extensive manual testing across the field types but have to admit that something like broken #states may be something we didn't specifically test for. Tagging for needs manual testing to do another round of manual testing for all field types.

We should improve the grouping of the fields in the edit form once we have landed this and #3386762: Use modals in field creation and field edit flow.

If #35 works this would be really great.

+++ b/core/modules/field_ui/tests/modules/field_ui_test_deprecated/field_ui_test_deprecated.module
@@ -0,0 +1,26 @@
+function field_ui_test_deprecated_form_field_storage_config_edit_form_alter(&$form, FormStateInterface $form_state) {
+  if (!($form_state->getFormObject() instanceof FieldStorageConfigEditForm)) {
+    throw new \LogicException('field_storage_config_edit_form() expects to get access to the field storage config entity edit form.');
+  }
+  if (!($form_state->getFormObject()->getEntity() instanceof FieldStorageConfigInterface)) {
+    throw new \LogicException('field_storage_config_edit_form() expects to get access to the field storage config entity.');
+  }
+
+  $form['hello'] = [
+    '#markup' => 'Greetings from the field_storage_config_edit_form() alter.',
+  ];
+}

Can we alter something on $form as well, i.e. not just add. So we can show that $form has the expected structure? Ie something that would have worked before and after this change? like maybe add something to the cardinality container?

Updated the issue summary to take into account the BC layer for the \Drupal\field_ui\Form\FieldStorageConfigEditForm form alters.

The MR is ready for another round of review.

Manually tested all the core field types again, and everything works as before.
- The #states related bug in Files displayed by default field in the FileItem storage settings is now fixed by 0c3d584e.
- Throughly tested the more complex field types such as entity reference, selection lists, file and image and those seem to work fine as well.
- The image field type has a Default image field appearing twice now that the forms are combined (one from each: field and field storage forms) which IMO unnecessarily makes the form lengthy. However, it is out of this issue's scope and can be removed from one of the forms in a follow-up.

Reviewed the newly added BC layer for the field storage form alter hook and the changes look good. Moving back to RTBC.

1)
    Drupal\Tests\media_library\FunctionalJavascript\FieldUiIntegrationTest::testFieldUiIntegration
    Behat\Mink\Exception\ExpectationException: Checkbox "set_default_value" is
    not checked, but it should be.
    
    /builds/project/drupal/vendor/behat/mink/src/WebAssert.php:794
    /builds/project/drupal/vendor/behat/mink/src/WebAssert.php:736
    /builds/project/drupal/core/modules/media_library/tests/src/FunctionalJavascript/FieldUiIntegrationTest.php:98
    /builds/project/drupal/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

The gitlabci pipeline is showing - is this a random fail caused by these changes?

I'm not sure #40 is caused by this but since it's close enough, I added wait that is targeting the screenreader announcement which should address this.

Reviewed all the changes since the end of September, and I'm +1 to this being RTBC.
I especially like the Subform changes, I think this marks the first core usage outside of plugins.

Thanks to everyone who worked on this, but especially @srishtiiee who carried it for so long, and to @lauriii and @alexpott for getting it to the end (I hope it's the end!)

I think there are two things left to do:
1. Add something about #states to the CR - if a field storage form in contrib or custom uses this it will need updating
2. Add / find the follow-up about the 2 default value sections on the image field.

1. Updated the CR to mention that modules may need to update selectors targeting certain attributes
2. Opened a new issue to tackle the image default value issue: #3392132: The option to define default image on both, storage and instance level is confusing

Let's get this in now so it'll be there when 10.2.x is opened. All the things that are changing are not part of the BC promise - form arrays and html and we've made an effort to provide BC layers for form alters.

Committed ea3f7b5 and pushed to 11.x. Thanks!

This introduced a very frequent random test failure: #3392878: EntityReferenceAdminTest fails often

Adding credit from #2524322: Users deleted a field instead of going back to change their settings which I closed as a duplicate.

Usability review

We discussed this issue at #3392045: Drupal Usability Meeting 2023-10-13. That issue will have a link to a recording of the meeting.
For the record, the attendees at today's usability meeting were @AaronMcHale, @anmolgoyal74, @benjifisher, @rkoller, @simohell, and @worldlinemine.

If you want more feedback from the usability team, a good way to reach out is in the #ux channel in Slack.

First of all, I want to thank everyone for their efforts on this issue, I know a lot of hard work has gone into this, and it's wonderful to see all of the improvements going into the Field UI, which it has desperately needed, and the improvements to the Field UI make me personally really excited for 10.2!

I also want to acknowledge that the group reviewed this after it was committed, so for the specific things the group identified follow-up issues should be created. Additionally, after this issue was reviewed at the meeting on 13 October at the usability meeting, @lauriii and I discussed this change at DrupalCon Lille. After our discussion I personally am now much clearer on what this issue is trying to address. So while the review at the usability meeting lacked some of that context, this comment today includes the additional context that @lauriii shared with me.

The changes that this issue introduces are generally positive, new users may find the two forms confusing and not understand why field settings are spread across two forms, so for fields which only have a single instance, meaning they are only used once, this is a positive change. However, it becomes a bit trickier when multiple instances of the field are used. This is where the first problem comes in.

A common theme across the problems that we saw was that because fields in the Storage Settings fieldset can have a wider impact and scope than the other fields on the form. In general we recommend that one form does not try to do too many things, and so we felt that overall a fieldset may not be the most appropriate pattern to use in this case.

#1 The single form now results in changes at different levels of the field system

What this means is that, say the node body field exists on each content type, if you change the label of the body field, that change applies only to that instance of the field, but if you change the number of allowed values from 1 to unlimited, that applies across all instances.

When those settings were on two clearly separate forms, there was a clearer separation between the field instance and storage settings. However, now with those on the same form, the user can change both of those settings in the same operation. It could be quite easy for the user not to see the help text under the storage settings fieldset, or not understand what it means for something to be changed at the storage level, and assume that changing both the help text and the number of allowed values impacts only that instance of the field, and so not realize the number of allowed values was changed for all instances of the body field.

Now, I'm not saying that it was very clear before, but the separation of those onto two forms meant that the user at least had to make those changes separately, and so there was a clearer separation.

#2 When navigating using only keyboard navigation, there is no clear distinction between settings which apply at the instance level and settings which apply at the storage level

Following on from the previous concern, a user navigating the form using only keyboard navigation may be completely unaware that some settings are saved at the instance level and some at the storage level.

When tabbing through the form, the following happens:

1. The user hits the label setting (instance),
2. Then tabs to the number of allowed values setting (storage)
3. Then to the help text setting (instance.

We can see that if the user is only tabbing through the form, they miss the context of the help text and fieldset. This is especially problematic for users who have no sight and are relying entirely on assistive technology. The user may also be using tools to simply jump to one setting anywhere in the form, in this case the user would have absolutely no idea if the setting they are changing is saved at the instance or storage level.

It also doesn't help that the Storage Settings fieldset is inserted inbetween instance-level settings.

A possible solution to this could be to borrow a pattern from the Translations interface, where the scope of the translation is included in the field label. (credit to @simohell for finding this pattern and the screenshot below).

In this example we see the "Name" element, with the text "all languages" in parenthesis next to the label.

An additional change could be made to ensure that the Storage-level settings are never mixed in with the instance settings. For example, displaying storage-level settings at the bottom of the form. Perhaps using the tab group/vertical tabs pattern that's used across other parts of Core, with "Storage settings" being the default tab.

#3 The storage settings fieldset is not visually distinct enough to convey the importance of the difference between storage level settings and instance level settings

Looking at the new Storage Settings fieldset as a whole. When using the Claro admin theme, fieldset only provides minor indentation and a grey border to encapsulate the storage level settings. Help text does exist inside the fieldset to inform the user that these settings apply at the storage level, but it would be incredibly easy to miss this help text because the user may just be scanning the elements on the form, or they may be tabbing and using assistive technology (as describe in the previous point).

Other admin themes may change the visual look of the fieldset and help text, making them more or less visible.

We were concerned that the fieldset does not provide enough distinction between the instance level settings and the storge level settings to convey the difference in their scope and impact.

This could be mitigated by the changes suggested in point 2, including by looking at using the vertical tabs pattern. Further iteration and testing may be needed to find the most suitable solution.

As I said, I think it makes sense to open up follow-up issues to go into these in more detail, whether that's one issue or multiple I'm not sure.

Thanks,
-Aaron, on behalf of the usability meeting