Provide better default configuration when re-using an existing field

Rerolled the latest patch for 10.1.x.

The last submitted patch, 77: 2717319-77.patch, failed testing. View results →

This should handle some of the test failures.

Addressing phpcs violations

The last submitted patch, 80: 2717319-80.patch, failed testing. View results →

Looks like I accidentally posted an earlier iteration of the patch.. This should address some of the test failures.

The last submitted patch, 82: 2717319-81.patch, failed testing. View results →

Since the thread is pretty long with several different ideas, I decided to test the current patch. I believe what this patch does is preset the manage form and manage display settings for a re-used field. I confirmed that the patch works great for that feature. This is a good improvement for image fields. I constantly forget to set the image style.

I do not have time to test, but I see this comment in the code:

+      // If $existing_bundle is not supplied, then find a suitable candidate.
+      // @todo Consistently choose the bundle. Oldest? Newest?

That is a serious usability question, and that is why this issue stalled back when I was working on it actively. In Comment #62, I suggested a different approach: offer a select list of bundles that already use the same field storage, and fill in configuration with JS when the user selects an option.

Interesting. I didn't test it with multiple types that use the same field with different settings.

#85: I removed the @todo comment and added sorting to make the behavior deterministic. I believe we should move discussing the option to select bundle to a follow-up. This issue in its current scope is already a net win.

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

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Addressing PHPCS violations.

Most of these are maybes or questions, I'm guessing the switch back to NR shouldn't take long 😎
.

1. Could we get a test only patch with testReuseField()?

2. +++ b/core/modules/field_ui/src/Form/FieldStorageAddForm.php
   @@ -566,6 +577,139 @@ protected function getExistingFieldLabels(array $field_names) {
   +   *   'entity_view_display' if these are defined for the existing field. If the
   

   Should this clarify "in an instance of the existing field"?

3. +++ b/core/modules/field_ui/src/Form/FieldStorageAddForm.php
   @@ -566,6 +577,139 @@ protected function getExistingFieldLabels(array $field_names) {
   +      if ($existing_bundle == $this->bundle) {
   

   Is the loose equivalence intentional here?

4. +++ b/core/modules/field_ui/src/Form/FieldStorageAddForm.php
   @@ -566,6 +577,139 @@ protected function getExistingFieldLabels(array $field_names) {
   +      if ($existing_bundle == $this->bundle) {
   +        return [];
   +      }
   +      if (empty($field_map[$this->entityTypeId][$field_name]['bundles'][$existing_bundle])) {
   +        return [];
   +      }
   

   Is there a benefit in combining the conditions since they both return an empty array? Maybe it's to benefit readability?

5. +++ b/core/modules/field_ui/src/Form/FieldStorageAddForm.php
   @@ -566,6 +577,139 @@ protected function getExistingFieldLabels(array $field_names) {
   +      $existing_bundle = reset($bundles);
   

   I was going to suggest a potentially better way to pick the ideal existing bundle, but I think that can wait to a followup as what is happening here is a net improvement already. Refining this approach could delay this welcome improvement from landing.

6. +++ b/core/modules/field_ui/src/Form/FieldStorageAddForm.php
   @@ -566,6 +577,139 @@ protected function getExistingFieldLabels(array $field_names) {
   +      'default_value' => $existing_field->getDefaultValueLiteral(),
   +      'default_value_callback' => $existing_field->getDefaultValueCallback(),
   

   I could see the default values being copied over potentially being of concern, it's probably worth an explanation why this is OK to do in anticipation of such concerns.

7. +++ b/core/modules/field_ui/src/Form/FieldStorageAddForm.php
   @@ -566,6 +577,139 @@ protected function getExistingFieldLabels(array $field_names) {
   +    $field_type_definition = $this->fieldTypePluginManager
   +      ->getDefinition($field_name);
   +    $options = $this->fieldTypePluginManager->getPreconfiguredOptions($field_type_definition['id']);
   

   The line break before ->getDefinition() seems odd since the next line has no breaks and is longer than if 677-678 were all on one line.

#91.1 👍
#91.2 👍
#91.3 👍
#91.4 We certainly could combine those rules but the if condition would become harder to read. IMO what is there currently is easier to read but I don't feel strongly about that.
#91.5 👍
#91.6 What are the concerns with copying over the default value? It seems confusing from the UX perspective if we would leave some of the configuration options out. The user is always taken to the configuration form when the instance is created, so if something is off, they have an opportunity to correct that.
#91.7 👍

The last submitted patch, 92: 2717319-92.patch, failed testing. View results →

#92 failed because of a known random failure.

I agree that we can remove the argument 👍

The patch looks pretty good! I only read the code and haven't tested it locally yet, so here's a few remarks for now:

1. +++ b/core/modules/field_ui/src/Form/FieldStorageAddForm.php
   @@ -442,25 +425,38 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   +   * @param array[] $widget_settings
   
   @@ -468,25 +464,40 @@ protected function configureEntityFormDisplay($field_name, $widget_id = NULL, ar
   +   * @param string[][] $formatter_settings
   

   The @param for these two doesn't seem to match, but they should...

2. +++ b/core/modules/field_ui/src/Form/FieldStorageAddForm.php
   @@ -442,25 +425,38 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   +   *   (optional) Array of widget settings, keyed by form mode. Defaults to [].
   
   @@ -468,25 +464,40 @@ protected function configureEntityFormDisplay($field_name, $widget_id = NULL, ar
   +   *   formatter. Defaults to [].
   

   I think we usually write (Defaults to) "an empty array" in documentation text, not the PHP syntax for it.

3. +++ b/core/modules/field_ui/src/Form/FieldStorageAddForm.php
   @@ -442,25 +425,38 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   +    // in other form modes  until it is explicitly configured. When an existing
   

   Double space between "modes" and "until" :)

4. +++ b/core/modules/field_ui/src/Form/FieldStorageAddForm.php
   @@ -442,25 +425,38 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   +    $form_modes = $this->entityDisplayRepository->getFormModes('node');
   
   @@ -468,25 +464,40 @@ protected function configureEntityFormDisplay($field_name, $widget_id = NULL, ar
   +    $view_modes = $this->entityDisplayRepository->getViewModes('node');
   

   This kinda shows that the patch was only tested with nodes :) Any chance we can also add tests for another entity type?

5. +++ b/core/modules/field_ui/src/Form/FieldStorageAddForm.php
   @@ -442,25 +425,38 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   +      $form = $this->entityDisplayRepository->getFormDisplay($this->entityTypeId, $this->bundle, $mode);
   

   Can we name this variable $form_display? The current name is very confusing.

6. +++ b/core/modules/field_ui/src/Form/FieldStorageAddForm.php
   @@ -442,25 +425,38 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   +      $this->messenger()->addMessage($this->t('The following form mode(s) were enabled: %enabled_modes', [
   
   @@ -468,25 +464,40 @@ protected function configureEntityFormDisplay($field_name, $widget_id = NULL, ar
   +      $this->messenger()->addMessage($this->t('The following view mode(s) were enabled: %enabled_modes', [
   

   It would be better if we'd use formatPlural() here.

   Also, should we exclude default from $enabled_modes? By only scanning the code in this patch, it seems the user would get (at least) 3 messages when adding a new field:

   • Your settings have been saved.
   • The following form mode(s) were enabled: Default
   • The following view mode(s) were enabled: Default

7. +++ b/core/modules/field_ui/src/Form/FieldStorageAddForm.php
   @@ -468,25 +464,40 @@ protected function configureEntityFormDisplay($field_name, $widget_id = NULL, ar
   +      $view = $this->entityDisplayRepository->getViewDisplay($this->entityTypeId, $this->bundle, $mode);
   

   Same here: $view -> $view_display.

8. +++ b/core/modules/field_ui/src/Form/FieldStorageAddForm.php
   @@ -558,6 +569,138 @@ protected function getExistingFieldLabels(array $field_names) {
   +   * @throws \Drupal\Component\Plugin\Exception\InvalidPluginDefinitionException
   +   * @throws \Drupal\Component\Plugin\Exception\PluginNotFoundException
   

   I can't see anywhere in this method a place where these exceptions can be thrown.

9. +++ b/core/modules/field_ui/src/Form/FieldStorageAddForm.php
   @@ -558,6 +569,138 @@ protected function getExistingFieldLabels(array $field_names) {
   +   *   The machine name of the field. The field class must implement
   +   *   Drupal\Core\Field\PreconfiguredFieldUiOptionsInterface.
   ...
   +    $field_options = $options[$preset_key];
   

   This requirement seems a bit artificial since getPreconfiguredOptions() will return an empty array if the field type class doesn't implement that interface.

   Looks like we can remove the "requirement" by defaulting $options[$preset_key] to an empty array.

10. +++ b/core/modules/field_ui/src/Form/FieldStorageAddForm.php
    @@ -558,6 +569,138 @@ protected function getExistingFieldLabels(array $field_names) {
    +   *   An array of settings with keys 'field_config', 'entity_form_display', and
    

    This method also returns a field_storage_config key.

Thanks @amateescu! This should address all of your feedback. 😊

After a lot of contemplating, I decided to remove the functionality that enables new view/form modes because even after spending time reading comments on this issue, it's hard to understand when I would expect that to happen. If we still think that's something we'd like to do, we should do that in a follow-up with some additional research to validate that it is what users expect.

This issue would be epic to land together with 🐛 Improve re-use an existing field user experience Fixed .

Did a bit of testing with the patch applied locally and it works great. Then went through the code one more time, found just a few variable names that could be more clear (similar to the $form -> $form_display point from #97), but it's not worth holding up this issue for them :)

I did another review of the code, but after the very thorough amateescu review this appear to be in great shape.

I have the 10.0.x patch available if there is interest in backporting. This is an objective improvement, but enough of a change that I'm hesitant to include it in a patch release. There's been enough times recently that I committed something to just 10.1.x then became aware of interest to backport that I figured having the patch at the ready in #101 would be a good idea.

This is now committed to 10.1.x. It's a really nice improvement, and always nice to see completion of an issue that was filed 2 months before the first season of Stranger Things premiered.

I did a little demo of this issue at 📌 Drupal Usability Meeting 2023-03-24 Needs work . It starts at about 17:17 in the recording (YouTube).

I showed the effect on the selected image style for an Image field and the form widget for a Paragraph (ERR) field.

This is a nice improvement. I don't think this is eligible for backport as it does make a change to the UI, but tagging so we can highlight it in 10.1.0 if other agree.

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