Move label and machine name fields to the field config edit form

Created on 30 October 2023, 9 months ago
Updated 4 June 2024, about 1 month ago

Problem/Motivation

The label and machine name are currently configured on the second step of the field creation process. This has been done purely for technical reasons, because \Drupal\field_ui\Form\FieldConfigEditForm currently expects to have an instance of \Drupal\field\FieldConfigInterface which requires that the machine name has been set.

From user experience perspective it would be ideal if we could avoid modals with a single form element. For example, when adding a boolean field, the second modal would have has just the label and machine name fields.

Proposed resolution

Move label and machine name fields to the field config edit form.
To achieve this we still need to create a dummy field instance with a random field_name in core/modules/field_ui/src/Form/FieldStorageAddForm.php which is updated at the end of field creation process in core/modules/field_ui/src/Form/FieldConfigEditForm.php.

Remaining tasks

User interface changes

FieldConfigEditForm

API changes

Data model changes

Release notes snippet

๐Ÿ“Œ Task
Status

Needs work

Version

11.0 ๐Ÿ”ฅ

Component
Field UIย  โ†’

Last updated about 15 hours ago

Created by

๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

Live updates comments and jobs are added and updated live.
  • Field UX

    Usability improvements related to the Field UI

  • Needs release manager review

    It is used to alert the release manager core committer(s) that an issue significantly affects the overall technical debt or release timeline of Drupal, and their signoff is needed. See the governance policy draft for more information.

  • Needs framework manager review

    It is used to alert the framework manager core committer(s) that an issue significantly impacts (or has the potential to impact) multiple subsystems or represents a significant change or addition in architecture or public APIs, and their signoff is needed (see the governance policy draft for more information). If an issue significantly impacts only one subsystem, use Needs subsystem maintainer review instead, and make sure the issue component is set to the correct subsystem.

  • Needs issue summary update

    Issue summaries save everyone time if they are kept up-to-date. See Update issue summary task instructions.

  • Needs change record

    A change record needs to be drafted before an issue is committed. Note: Change records used to be called change notifications.

Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @lauriii
  • ๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

    This was unblocked by ๐Ÿ“Œ Make field selection a two-step form Fixed .

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States benjifisher Boston area

    Just to clarify: if the first step of the "Add field" form selects a field type (like Boolean) and not a field group (like Reference), then we should skip the second step and go straight to the field configuration form.

    Maybe we should move some code from the submit method of the "Add field" form to the buildForm method of the field configuration form. I am not sure how that would affect the "Re-use an existing field" form.

  • First commit to issue fork.
  • Merge request !5992Move label to FieldConfigEditForm โ†’ (Open) created by srishtiiee
  • Pipeline finished with Failed
    6 months ago
    Total: 615s
    #70486
  • Pipeline finished with Failed
    6 months ago
    #74166
  • Status changed to Needs work 6 months ago
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia srishtiiee

    The second step in case of ungrouped field types mentioned in #3 isn't implemented yet. It will be affected by the issue that removes the radio buttons from the field cards in the first step: ๐Ÿ“Œ Field selection breaks conventions and increases cognitive load Needs review . I think we should wait on those changes to be committed to avoid re-work here.

  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia omkar.podey

    omkar.podey โ†’ made their first commit to this issueโ€™s fork.

  • Pipeline finished with Failed
    6 months ago
    Total: 590s
    #75457
  • Pipeline finished with Failed
    6 months ago
    Total: 540s
    #76251
  • Pipeline finished with Failed
    6 months ago
    Total: 162s
    #77475
  • Pipeline finished with Failed
    6 months ago
    Total: 865s
    #77607
  • Pipeline finished with Failed
    6 months ago
    Total: 718s
    #77618
  • Pipeline finished with Failed
    6 months ago
    Total: 576s
    #78101
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia omkar.podey

    The ajax callback was causing the form to be rebuilt which prevented it from being saved the first time, now fixing the issue with the correct order of execution as we should run the validation checks earlier and not let the temp field to be saved early.

  • Pipeline finished with Failed
    6 months ago
    #78527
  • Pipeline finished with Failed
    6 months ago
    Total: 570s
    #78635
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia omkar.podey

    Now the tests are failing because of two main reasons.

    1. The content translation module somehow doesn't appear to be installed, while works fine in UI.
    2. The default value field is created with the random temp field name so have to figure out a way to assert that.
  • Pipeline finished with Failed
    6 months ago
    Total: 238s
    #79821
  • Pipeline finished with Failed
    6 months ago
    Total: 238154s
    #79822
  • Pipeline finished with Failed
    6 months ago
    Total: 226s
    #80605
  • Pipeline finished with Failed
    6 months ago
    Total: 694s
    #80623
  • Pipeline finished with Failed
    6 months ago
    #80694
  • Pipeline finished with Failed
    6 months ago
    #80850
  • Pipeline finished with Failed
    6 months ago
    #81207
  • Pipeline finished with Running
    6 months ago
    #81231
  • Pipeline finished with Success
    6 months ago
    #81239
  • Pipeline finished with Success
    6 months ago
    #81844
  • Status changed to Needs review 6 months ago
  • Status changed to RTBC 6 months ago
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia Kanchan Bhogade

    Hi,
    I have tested MR !5992 on the Drupal version 11.x

    Testing steps:

    1. Install Drupal 11.x
    2. Go to any content type and add a field type like boolean (not a field group like Reference)
    3. Check for the 2nd step having the Label and machine name
    4. Apply branch and again check for add field

    Test Result:
    The patch is applied successfully, and the second step form is skipped and directly goes to the field configuration form for the Boolean type field which does not have a field group like Reference.

    Attaching screenshots for reference.

    Moving to RTBC

  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany rkoller Nรผrnberg, Germany

    hm i've tried to apply the patch to a completely fresh install of drupal 11.x-dev installed with the standard profile. but somehow i get Could not apply patch! Skipping. The error was: Cannot apply patch https://git.drupalcode.org/project/drupal/-/merge_requests/5992.diff when i try to apply the diff with composer patches. but "somehow" some" changes or all(?) changes got applied. but not sure if that is an actual problem with the patchor a problem due to the way composer-patches applies the patch, just wanted to note.

    but changes are in place, at least in the UI, so i was able to give it a quick try. definitely the right choice moving the label/machine_name field over to the field config edit form. looks great!

    one detail to note. ๐Ÿ“Œ [PP-1] Replace the "Change field type" button with one that goes to the previous step Postponed which is currently postponed on this issue is a sort of essential addition to this issue from my perspective. because currently the only way to move one step back as soon as you are on the field config edit form is the back button of your browser. there should definitely be a dedicated option to navigate back and forth in the interface through all the steps until the field is finally saved in this last step. otherwise one might feel stuck arriving on this field config edit form in case the person had clicked the wrong field type or option in one of the previous steps.

  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany rkoller Nรผrnberg, Germany

    Another detail I've just noticed. For the field type Media you directly get to the field settings form now. "problem" is like Field upload pre patch it had a description underneath explaining when to use media reference fields and when to use file and image reference fields. For Field upload that description is still shown on step two when you are able to choose the option, that second step got removed for Media and so is the description. I am not sure if it would make sense to squeeze it onto the field settings page instead for media. but i wonder if it would make sense to create a dedicate help topics page in a follow up where the user gets an overview of all available fields in core and when best to use which field. that you have all available options and background information for each field type on a single page.

  • Status changed to Needs work 6 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States benjifisher Boston area

    @Kanchan Bhogade: Thanks for testing, but please do not move an issue to RTBC unless it has also passed code review. The R is for review and the T is for test.

    @rkoller: I am moving the issue back to NW for Comment #13. Losing the notice about media and image fields is a regression. I am adding a note to the issue summary so that we do not lose track of this regression.

    I do not think we have to worry about applying the diff as a patch (Comment #12). You should be able to clone the issue fork and check out the feature branch using the instructions near the top of the issue,

    I think it is OK to keep ๐Ÿ“Œ [PP-1] Replace the "Change field type" button with one that goes to the previous step Postponed as a separate issue. If we can get this issue fixed, then I think it is a step in the right direction even without that improvement. I hope we can get both done in time for Drupal 10.3.

  • ๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

    I am moving the issue back to NW for Comment #13. Losing the notice about media and image fields is a regression. I am adding a note to the issue summary so that we do not lose track of this regression.

    I'm not 100% sure the message is needed for the media type at least when not editing fields for media entity. Maybe on this issue we move the message to the field config edit form and discuss in a follow-up if we should not display it when editing fields for other entity types than media.

  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany rkoller Nรผrnberg, Germany

    I am not sure if the field settings edit form is the best place even as a temporary place for that description. That form and page is the last step in the field creation flow, step 2 for media and step 3 for file upload with this patch applied. that description ideally would be more useful at the point where the person trying to create a field and either doesn't know anything about the different field types or is uncertain which field type is the right pick, in case of the media and file upload example, and needs a reminder.
    i wonder if it would make sense to remove both the descriptions for media and file upload in step 2 and 3. instead incorporate the brief gist into their short descriptions in step one (i am currently working on a draft for ๐Ÿ“Œ Refine field descriptions Active ). in addition to that provide a description pointing to a to be created help topic page providing an overview for a "when to pick which field type" situation. But I would place the description pointing to the help topics page before the list of available field types in step 1 instead of after.

  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia omkar.podey

    Then maybe something like this

    could work, just having the general message appear on the initial field selection whenever media is enabled.

  • Pipeline finished with Failed
    5 months ago
    Total: 197s
    #88617
  • Pipeline finished with Success
    5 months ago
    Total: 544s
    #88656
  • Status changed to Needs review 5 months ago
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia djsagar

    After applied MR !5992, Message appear bottom of the choose field type.

    Before MR

    After MR

    RTBC ++.

  • Status changed to Needs work 5 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States bnjmnm Ann Arbor, MI

    I'm not sure about adding info to the bottom of this form specific to choosing media. The options themselves already include description text specific to that option - and option-specific descriptions should live there. Ideally the current media description text could be shortened (its longer than it needs to be IMO) to make room for this info. Having it as a footnote technically addresses the regression, but I don't think it actually improves the experience.

    If rewriting that description is not a feasible option, at the very least associate the bottom-of-the-form text with the media option with an asterisk. In its current state I think it is unlikely a user will even read it - they will see the option they want then click it to proceed.

  • Pipeline finished with Failed
    5 months ago
    Total: 171s
    #94575
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia omkar.podey

    Does this seem better and attracts enough attention to the note?

  • Status changed to Needs review 5 months ago
  • Pipeline finished with Failed
    5 months ago
    Total: 623s
    #94737
  • Pipeline finished with Success
    5 months ago
    Total: 570s
    #94754
  • Status changed to Needs work 5 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States bnjmnm Ann Arbor, MI

    Does this seem better and attracts enough attention to the note?

    It's better than not having the association, but it's not great. I think updating the descriptions has a better result as it keeps the UI clean, removes the edge case, and the relevant information is in the correct context. Also less code needed ๐Ÿ™‚

    Something like this looks better and I think has a higher chance of benefitting a user vs. the current tacked on message.

  • Pipeline finished with Failed
    5 months ago
    Total: 500s
    #95545
  • Pipeline finished with Failed
    5 months ago
    Total: 139s
    #95695
  • Pipeline finished with Failed
    5 months ago
    Total: 1038s
    #95701
  • Pipeline finished with Success
    5 months ago
    Total: 1580s
    #95716
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia omkar.podey

    Updated descriptions, more suggestions ?

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States phenaproxima Massachusetts

    This stuff is really tricky to understand, so nice work @omkar.podey! That said, I did a code review and found some stuff that should probably be fixed, plus some questions that need answering.

  • Pipeline finished with Failed
    5 months ago
    Total: 515s
    #96660
  • Pipeline finished with Failed
    5 months ago
    Total: 860s
    #96678
  • Pipeline finished with Success
    5 months ago
    Total: 2576s
    #98632
  • Pipeline finished with Failed
    5 months ago
    Total: 544s
    #98722
  • Pipeline finished with Canceled
    5 months ago
    Total: 10s
    #99331
  • Pipeline finished with Failed
    5 months ago
    Total: 169s
    #99332
  • Pipeline finished with Failed
    5 months ago
    #99362
  • Pipeline finished with Failed
    5 months ago
    Total: 2045s
    #99392
  • Pipeline finished with Failed
    5 months ago
    Total: 1140s
    #99457
  • Pipeline finished with Failed
    5 months ago
    Total: 2074s
    #99488
  • Pipeline finished with Success
    5 months ago
    Total: 672s
    #99529
  • Status changed to Needs review 5 months ago
  • Pipeline finished with Success
    5 months ago
    Total: 572s
    #100356
  • Pipeline finished with Success
    5 months ago
    Total: 538s
    #104659
  • Pipeline finished with Success
    5 months ago
    Total: 491s
    #104861
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    See a lot of conversation in the threads and feedback (60 threads!) This good for review now?

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States tim.plunkett Philadelphia

    I've asked @omkar.podey to mark resolved threads with โœ… so @srishtiiee can close them

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    Appreciate it!

  • Pipeline finished with Success
    4 months ago
    Total: 936s
    #110059
  • Pipeline finished with Success
    4 months ago
    Total: 472s
    #110350
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    So testing was pretty straight forward. Applied the MR and verified label is appearing on config edit form.
    Tested with a few field types (had 0 doubt this would matter but double checked) and no issue creating fields.
    Remaining tasks appear to be complete.
    Issue summary contains a screenshot of the new change.

    +1 from me but may be worth getting additional eyes.

  • Pipeline finished with Failed
    4 months ago
    Total: 531s
    #113031
  • Pipeline finished with Success
    4 months ago
    Total: 534s
    #113062
  • Pipeline finished with Success
    4 months ago
    Total: 469s
    #113706
  • Pipeline finished with Success
    4 months ago
    Total: 490s
    #113729
  • Status changed to RTBC 4 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    From what I can tell all feedback has been addressed.

  • Status changed to Needs review 4 months ago
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia omkar.podey

    There are still unresolved threads.

  • Pipeline finished with Failed
    4 months ago
    Total: 544s
    #122034
  • Pipeline finished with Canceled
    4 months ago
    Total: 24s
    #122043
  • Pipeline finished with Success
    4 months ago
    Total: 517s
    #122044
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    Appears to be a few open threads or is this ready for testing?

  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia omkar.podey

    yep it is ready to be tested

  • Pipeline finished with Success
    3 months ago
    Total: 636s
    #135537
  • Status changed to RTBC 3 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    Rebased the MR as thought it had been a while, nothing broke

    Retested following same steps

    Applied the MR and verified label is appearing on config edit form.
    Tested with a few field types (had 0 doubt this would matter but double checked) and no issue creating fields.

    Believe all threads have been fixed or answered

    For me this looks good.

  • Status changed to Needs work 3 months ago
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch

    There are unresolved threads on the MR for things that are very confusing to me.

  • Pipeline finished with Success
    3 months ago
    Total: 572s
    #137253
  • Status changed to Needs review 3 months ago
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia omkar.podey

    I simplified the whole code block by removing the creation of new field storage config and hence moving away from the whole translatable key issue.

  • Pipeline finished with Success
    3 months ago
    Total: 651s
    #138453
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    Seems to be 1 open question left a comment. Once answered don't mind marking

  • Status changed to RTBC 3 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    Keep this one from stalling going to mark as it's still functional.

  • Status changed to Needs work 3 months ago
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch

    Replied on the MR - but this shouldn't have been RTBC again, the thread is still outstanding.

  • Pipeline finished with Failed
    3 months ago
    Total: 175s
    #157080
  • Pipeline finished with Failed
    3 months ago
    Total: 182s
    #157121
  • Status changed to Needs review 3 months ago
  • Status changed to Needs work 3 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    MR appears to have some failures.

  • Pipeline finished with Success
    2 months ago
    Total: 566s
    #159579
  • Status changed to Needs review 2 months ago
  • Status changed to RTBC 2 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    Appears feedback has been addressed.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States xjm

    With this patch applied, I'm getting a weird behavior where clicking "Save" on the field configuration form the first time causes the page to jump back to the top, and I have to click "Save" a second time in order to save the configuration. It does not seem to happen with HEAD. Steps to reproduce:

    1. Install Standard on 11.x HEAD with the patch/MR applied.
    2. Go to admin/structure/types/manage/article/fields
    3. Click "Create a new field".
    4. Select "Boolean" (it occurs for other field types, but this one's simplest)
    5. Enter a label.
    6. Click "Save". Note that the cursor jumps to the top of the page.
    7. Click "Save" a second time and note that the field is only then saved.

    I've been able to reproduce this consistently with the patch, and able to consistently reproduce it not happening with HEAD.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    Actually seeing the same as @xjm appears the code that runs the machine name could be causing it.

    If I enter a label
    Click out of the label input (any where on the page)
    Click save I don't get that behavior.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States xjm

    I also disagree that losing this message is an acceptable regression or scopable to a followup:

    This message was an important compromise for the stabilization of Media, to dissuade people from creating a bunch of difficult-to-migrate image data when they should have been using Media. That single paragraph was the compromise for giving up built-in redirects, a migration tool, removal of image fields from the UI except on Media types, etc. etc. etc. It does not have the greatest UX -- ideally it would be styled as a little info box rather than a blah paragraph of text, or something even more assertive -- but that doesn't mean it's okay to lose it entirely.

  • Status changed to Needs work 2 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States xjm

    I confirmed what @smustgrave said above: If you click out of the label field, the bug does not happen. (Clicking in or out of the label field in HEAD had no effect; it worked every time.)

    I also reproduced #47 in Firefox, both it not happening in HEAD, and it happening with the patch applied.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States xjm

    Another regression is in the page title for the field configuration page. In HEAD we have:

    With the patch it's simply:

    This sounds like it's all field settings for articles, rather than the settings of one particular field.

    I understand that we don't have the field name when the page is loaded, but could we at least put the field type in the title? E.g.:

    Boolean field settings for Article

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States xjm

    I finally finished my manual testing and my code review -- phew! Overall this is in pretty good shape, and it's a really good usability improvement on the whole. IMO there are three regressions that need addressing (see #47, #49, and #51), plus a few small cleanups, but on the whole it's close to ready. Great work everyone!

    Saving issue credits.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States xjm

    Tagging per my comment above about the temporary label and machine name being generated to make the form work.

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch

    Re #53 I don't have a better idea, but I found that incredibly confusing reading the MR (like deleting config entities that would never actually exist etc.). Definitely could use a follow-up to revisit. I haven't reviewed this since my last round of feedback was addressed.

  • Pipeline finished with Failed
    2 months ago
    Total: 173s
    #160738
  • Pipeline finished with Failed
    2 months ago
    Total: 172s
    #162284
  • Pipeline finished with Success
    2 months ago
    Total: 507s
    #162354
  • Pipeline finished with Failed
    2 months ago
    Total: 181s
    #163404
  • Pipeline finished with Failed
    2 months ago
    Total: 793s
    #163430
  • Pipeline finished with Failed
    2 months ago
    Total: 309s
    #163451
  • Pipeline finished with Success
    2 months ago
    Total: 908s
    #165259
  • Pipeline finished with Success
    2 months ago
    Total: 501s
    #165318
  • Status changed to Needs review 2 months ago
  • Status changed to Needs work 2 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States benjifisher Boston area

    The screenshot in the issue summary needs an update. The page title now includes the field type. For example, when adding a boolean field, the page title is "Boolean settings for Basic page Add to Default shortcuts", not "Field settings for Basic page Add to Default shortcuts".

    After I save the new field, I can still edit it. When I do that, the page title still includes the field type (like "Boolean") instead of the field name (like "Test boolean field"). The same thing happens with existing fields. For example, with the Standard profile I can visit /admin/structure/types/manage/page/fields/node.page.body, and I see the page title "Text_with_summary settings for Basic page". I think we should use the field name when it is available.

    I also left some comments on the MR. We need the usual BC steps when adding a parameter to the constructor, and that means we need a change record. I am adding the issue tag for that.

  • Pipeline finished with Failed
    about 2 months ago
    Total: 589s
    #175072
  • Pipeline finished with Failed
    about 2 months ago
    Total: 869s
    #176916
  • Status changed to Needs review about 2 months ago
  • Status changed to Needs work about 1 month ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    Looking at the tags and can't answer the release or framework manager. But can the issue summary be updated from #56 (should help the manager review later).

    Left a small comment on MR. Also appears to have a number of test failures. Some of them could need a rebase but didn't check each.

Production build 0.69.0 2024