Problem/Motivation

On admin/config/regional/date-time/formats/add only the Format string field has the required label the name field is missing that. if you fill out only the format string and language select box and save the required machine readable name field shows up.
date format machine readable name field is showing up after only formatstring and language was set and saved.
It is the same for /admin/structure/display-modes/form/add and /admin/structure/display-modes/view/add.

Steps to reproduce

- go to admin/config/regional/date-time/formats/add
- add something to the format string field eg m/d/Y - h:i a
- set the language in the select box and save

- now enter something in the name field -> the entry isnt added to the machine readable name as well in that state
- clear the name field again and enter something in the machine readable name field and save instead.
- edit the date format with no name. the machine readable name field isnt shown as a field anymore but as a string as usual.

Proposed resolution

I've created an initial merge request adding '#required' => TRUE to the $form['label'] fields in entitydisplaymodeformbase.php and dateformatformbase.php. more or less all other name/label fields in Core adding anything have the name/label field required. by requiring it is ensured the name field is filled and that way the at first invisible machine name field gets populated accordingly.

Remaining tasks

I am not sure if it would be necessary to add tests as well, i am not a developer and never dealt with tests before.

CommentFileSizeAuthor
#9 3273102-9.patch2.04 KBsmustgrave
#9 3273102-9-tests-only.patch949 bytessmustgrave
#7 after_#5_addview.png28.43 KBsonam.chaturvedi
#7 after_#5.png51.19 KBsonam.chaturvedi
#7 beforepatch.png57.15 KBsonam.chaturvedi
date_format.png68.57 KBrkoller

Issue fork drupal-3273102

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

rkoller created an issue. See original summary.

rkoller’s picture

rkoller
he/him
Primary language German
Location Nürnberg, Germany
commented
Issue summary: View changes

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

rkoller’s picture

rkoller
he/him
Primary language German
Location Nürnberg, Germany
commented
Title: Name field on the add date format page misses a required label » Name field on the add date format, format mode and view mode pages needs a required label
Issue summary: View changes
Issue tags: +Usability

rkoller opened merge request !2685

rkoller’s picture

rkoller
he/him
Primary language German
Location Nürnberg, Germany
commented
Status: Active » Needs review

and i'll set the issue to needs review. as noted in the issue summary i am not a developer and therefor not sure if a test is needed and in case it is how to write one (never done that).

oh and i notice initially the scope of the issue was about the date format but now got extended to the form mode and view mode as well. not sure if the component language system is the right fit anymore or if it was in the first place at all. :/ please change the component accordingly. thank you.

sonam.chaturvedi’s picture

sonam.chaturvedi commented
StatusFileSize
new beforepatch.png57.15 KB
new after_#5.png51.19 KB
new after_#5_addview.png28.43 KB

Verified and tested he MR !2685. Patch applied successfully on 9.5.x-dev.

Test Steps:
1. Goto admin/config/regional/date-time/formats/add
2. Check 'name' field is required.
3. Add something to the format string field eg m/d/Y - h:i a
4. Set the language in the select box and Save
5. Date format should be added successfully.
6. Repeat step 2 for /admin/structure/display-modes/form/add and /admin/structure/display-modes/view/add.

Test Results:
'name' field is required for :
1. /admin/config/regional/date-time/formats/add
2. /admin/structure/display-modes/form/add
3. /admin/structure/display-modes/view/add

Before patch:
before MR #5

After patch:
after MR#5
after patch

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs tests, +Bug Smash Initiative

Will need a test case.

smustgrave’s picture

smustgrave commented
Status: Needs work » Needs review
Issue tags: -Needs tests
StatusFileSize
new 3273102-9-tests-only.patch949 bytes
new 3273102-9.patch2.04 KB

Here's a tests-only patch and a copy of the MR + the test. Can move that test into the MR if you like. I only uploaded it so when the tests-only fails it doesn't move the ticket back to NW.

The last submitted patch, 9: 3273102-9-tests-only.patch, failed testing. View results

rkoller’s picture

rkoller
he/him
Primary language German
Location Nürnberg, Germany
commented
Status: Needs review » Needs work

oh thanks for the test! awesome that way i guess i was able to slightly grasp how things are intended to work. about the changes in #9 two questions.

1) is it correct that $this->assertSession()->elementAttributeExists('css', '#edit-date-format-pattern', 'required'); wasn't directly part of the issue but it looks like a test for that date format required pattern was missing so you have just added it along the way? (*that is just a question for my own understanding)

2) i guess the field in the field_ui module also needs a test for the required field in EntityDisplayModeFormBase.php? are the functional tests for that placed in EntityDisplayModeTest.php? based on the tests in DateTimeTest.php i would guess either one or both of the following code blocks have to be added? 

$this->drupalGet('admin/structure/display-modes/form/add/contact_message');
$this->assertSession()->elementAttributeExists('css', '#edit-label', 'required');
$this->drupalGet('admin/structure/display-modes/view/add/block_content');
$this->assertSession()->elementAttributeExists('css', '#edit-label', 'required');

or would be one of the two enough because they are based on the same file? or is it the other way around and every add case path has to be called with drupalGet line?

and about the how to proceed i dont mind. we could either stay with patches if it is easier that way or move everything into the MR. however you prefer. i'll set the issue to needs work. in case question 2 applies then the test has to be added but if not the issue could be set to RTBC i suppose? cuz the patch in #9 applied cleanly.

smustgrave’s picture

smustgrave commented

#11.1 that's correct. I couldn't find where it was testing that field was required also so added that test coverage
#11.2 Not sure about the field_ui. My understanding was this fix was directly for the add date format page.

You don't have to add drupalGet if you are staying on the same page while testing.

We can add back to the MR. The comment should probably be updated to "Verify name and format fields are required"
Only reason I added them as a patch was to get that red/green pattern. So the tests prove that your MR changes will fix the problem. I just haven't figured the best approach to get that red/green with MR yet.

rkoller’s picture

rkoller
he/him
Primary language German
Location Nürnberg, Germany
commented

#11.1 thanks for the confirmation and good choice
#11.2 the initial issue title, scope and report was about the date format page. but then i've realized when going through the add forms on a site again that there were more occasion and it applies to the form and view mode add pages as well, so i've rephrased the issue title and scope in #4 plus updated the issue summary.

well for the add pages for form modes you dont have a single page. you have: 

/admin/structure/display-modes/form/add/block_content
/admin/structure/display-modes/form/add/comment
/admin/structure/display-modes/form/add/contact_message
/admin/structure/display-modes/form/add/media
/admin/structure/display-modes/form/add/node
/admin/structure/display-modes/form/add/taxonomy_term
/admin/structure/display-modes/form/add/user
/admin/structure/display-modes/form/add/workspace

and the same amount of different pages for view-mode add pages. but each of the sixteen pages is based on EntityDisplayModeFormBase.php. thats why i've asked how to handle that case. if one drupalGet would be enough, if one drupalGet for display and one drupalGet for form mode add page would necessary, or each of those sixteen pages has to get a drupalGet?

and i agree about updating the comment including the format field.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.