Remove sample date from date field error message and title attribute

Actioned #74 and rerolled against 10.1.x. The issue summary looks good to me as well.

Re #76 and #79 - while the description for the created date has indeed been changed, this ticket is still removing it from other areas - the input's title and error message.

Updated patch with stan baseline fix.

Screenshots:

Before:

After:

@acbramley: I am a bit confused. Does this issue require more work or is it RTBC?

@xmacinfo no, it needs reviewing. It was set to Needs Work in #74 based on the deprecation error. That has been fixed. However, it's also tagged for usability and accessibility review, however I'm not 100% sure if that is required given it's a fairly straight forward change.

@acbramley: Thanks for the clarification. Let’s see if we can have usability and accessibility reviews.

Usability review

We reviewed this issue at 📌 Drupal Usability Meeting 2023-03-17 Fixed .

That issue will have a link to the recording, for the record the attendees were: myself, @rkoller, @simohell, @shaal and @blackbamboo.

After discussing various options, the group made the following recommendations:

1. We recommend [field_name] value is incorrect. Enter a valid date and time. for the error message text. For example: "Authored on value is incorrect. Enter a valid date and time".
2. We also recommend completely removing the title attributes.
3. In a follow-up issue, we should explore making the validation specific to the individual date or time elements, right now if the error is triggered by the time element, both the date and time elements are highlighted in red. This is not ideal, and we should find a way to highlight the specific element that is causing the error.
4. We used the "Authored on" Node field to test with, and we noted that the description of that filed could be improved, it simply says "The time that the node was created.", we felt this could be improved by saying "date and time", we also try to avoid the term "node" in the user interface and instead prefer "content". Obviously that is out of scope for this issue, but as with point three it would be useful if a follow-up issue could be filed for that.

Adding the needs followup tag for points 3 and 4.

Thanks @AaronMcHale!

Re 1. - This actually already is the error message (for the Authored On field at least). For other date time fields added to the content type there is no #title attribute on the $element since that comes from the fieldset itself. There is no other way I can see to get the title for those fields, this should be investigated in a follow up imo as it could have large flow on affects for getting that info in there.

The error messages in #80 were when using one of those custom fields (i.e a Date time field added to the Article content type). When testing the Authored on field, it shows correctly:

The problem with the title is already a bug in the current error message as well. Since this issue is just about removing the format I think that should be moved to a follow up.

I've removed the title attributes per #84.2.

So looks like we need 3 follow ups?

1. Improve Datetime validation to be specific to Date or Time input
2. Improve Authored On description (fwiw this is manageable using base field overrides although most people wouldn't be interacting with that system.
3. Add field title to Datetime validation error for configurable datetime fields.

Back to NR

@acbramley Thanks for the reply.

The text that is being proposed is [field_name] value is incorrect. Enter a valid date and time., which is different from what is in the patch in this issue. Note that we have specifically removed the word "invalid".

We probably then should have a follow-up issue to, as you say, remove the title attribute on the fieldset.

So it would be Authored on value is incorrect. Enter a valid date and time? Honestly that reads much worse than what's there currently to me.

To be clear - it's not a title attribute on the fieldset. When a non-base date field is rendered on the form it is wrapped in a fieldset. That fieldset contains the field's label in its legend, it is not a label on the element passed in (i.e not in $element['#title']. Therefore it is not possible to include that label in the error message (that I can see)

Not 100% sure what needs accessibility review. If general direction can be pointed out I'm not the worst 508 tester.
Moving to NW for all the follow up tickets being discussed.

Because of the recent changes in 10.1.x, the patch in #85 does not get applied. So just re-rolling it to be compatible with 10.1.x

please review.

please review.fixed CCF.

The last submitted patch, 93: 2791693-93.patch, failed testing. View results →

+++ b/core/lib/Drupal/Core/Datetime/Element/Datetime.php
@@ -374,7 +369,7 @@ public static function validateDatetime(&$element, FormStateInterface $form_stat
+          $form_state->setError($element, t('The %field date is invalid. Please enter a date in the valid format.', ['%field' => $title]));

Why has this changed from "in the correct format" to "in the valid format"? Please also post interdiffs with your changes and reasoning behind them.

I've fixed an issue in tests. Here is a patch to review.

D11 patch where Datetime::formatExample() was completely removed

"Please" is a forbidden word, so here is a new patch for D11

The last submitted patch, 97: 2791693-d10-97.patch, failed testing. View results →

The last submitted patch, 99: 2791693-d11-99.patch, failed testing. View results →

Ok, need to upload test only patch + a full patch

The last submitted patch, 102: 2791693-d10-102-TEST-ONLY-FAIL.patch, failed testing. View results →

The last submitted patch, 102: 2791693-d10-102.patch, failed testing. View results →

Ah, there was issue in the test, I did some cleanup, should be good now

The last submitted patch, 105: 2791693-d10-105-TEST-ONLY-FAIL.patch, failed testing. View results →

Whoop whoop, not it's time for D11 attempt

The last submitted patch, 107: 2791693-d11-107-TEST-ONLY-FAIL.patch, failed testing. View results →

IS mentions Deprecate DateTime::formatExample but didn't see in the patch

Also was previously tagged for follow ups have those been opened?

Hi @smustgrave,

Patch #105 for drupal 10 has a deprecation.
Patch #107 for drupal 11 does not have deprecation because the method was completely removed.

Am I missing something?

11.x is the current development branch that D10 releases will be cut from. So the deprecation needs to be included.

@smustgrave thanks, I missed that momoment, here is updated patch.

Looks good to me

There were a number of follow ups being discussed in 84-88 have those been opened?

Follow ups created

📌 [PP1] Improve Datetime validation to be specific to Date or Time input Active
📌 Improve Authored On description Active
📌 Add field title to Datetime validation error for configurable datetime fields Active

Needs accessibility review was added a long time ago (talked about PHP and JS changes which doesn't apply anymore), I don't feel like this new solution needs that anymore. It's also been through a usability review already.

Thanks @acbramley for cleaning those tags up!

Everything looks good but can deprecation be updated for 10.2 vs 10.1 please.

Then think it's good to RTBC!

Please review.

I'm triaging RTBC issues → . I read the IS and the comments. I didn't find any unanswered questions or other work to do.

Thanks for making this review easy by having an up to date IS and screenshots.

Thanks for making the followup issues, #115. I think should be siblings of this one so they are easy to find and to consider the 'big picture' That is, they should have the same parent as this one.

I read the patch, not a code review.

+++ b/core/lib/Drupal/Core/Datetime/Element/Datetime.php
@@ -5,8 +5,8 @@
+use Drupal\Core\Form\FormStateInterface;

The re-sorting is out of scope.

1. +++ b/core/lib/Drupal/Core/Datetime/Element/Datetime.php
   @@ -389,8 +384,14 @@ public static function validateDatetime(&$element, FormStateInterface $form_stat
   +   * @deprecated in drupal:10.1.0 and is removed from drupal:11.0.0. HTML date
   +   *   input format should not be exposed to users, as browser widgets expose
   +   *   input differently, varying by vendor, locale, device type, etc.
   

   By practice, this is the same deprecation message as in the trigger_error. So, lets keep them the same.

   @deprecated in drupal:10.2.0 and is removed from drupal:11.0.0. There is no replacement.

2. +++ b/core/lib/Drupal/Core/Datetime/Element/Datetime.php
   @@ -389,8 +384,14 @@ public static function validateDatetime(&$element, FormStateInterface $form_stat
   +   * @see https://www.drupal.org/project/drupal/issues/2791693
   

   There should be a blank line before the @see.

3. +++ b/core/lib/Drupal/Core/Datetime/Element/Datetime.php
   @@ -389,8 +384,14 @@ public static function validateDatetime(&$element, FormStateInterface $form_stat
   +   * @deprecated in drupal:10.1.0 and is removed from drupal:11.0.0. HTML date
   ...
   +    @trigger_error(__METHOD__ . ' is deprecated in drupal:10.1.0 and is removed from drupal:11.0.0. No replacement provided. https://www.drupal.org/project/drupal/issues/2791693', E_USER_DEPRECATED);
   

   This is close, but does not meet coding standards, it needs 'See'. And the version needs to be updated. Also changing the middle sentence to be consistent.

   is deprecated in drupal:10.2.0 and is removed from drupal:11.0.0. There is no replacement. See https://www.drupal.org/project/drupal/issues/2791693

   See @trigger_error() format →

@rpayanm, this issue has been using a patch work flow for many years and can continue to do so. There is no need to create an MR.

Finally, removing tags per the guidelines and setting back to NW.

Addresses #123

Added CR and updated messages.

@acbramley, thank you for making the changes.

I have reviewed the changes and read the CR and setting back to RTBC.

Adding some issue credit.

catch → credited BlackBamboo → .

catch → credited rkoller → .

catch → credited shaal → .

catch → credited simohell → .

Committed 55efc8e and pushed to 11.x. Thanks!

Did my best with issue credit but this is a very long issue so apologies if any omissions.

Published the CR

Just rerolled a patch for 9.5.11, without tests

Patch for 10.1.x branch to fix the issue.

@HitchShock @kavbiswa I don't think there is a plan to port this change to 10.x and 9.x? But I could be wrong.

Hi @jonathan1055
Sure, the solution will be released in the last Drupal version. We added these patches only for those who want to apply it to the older versions

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

Can someone please reroll the patch for Drupal 10.2.0?

@RichardDavies no need, the patch was committed to 11.x before 10.2.0 was released and is therefore available in 10.2.0 :)

@acbramley Ah, thank you for the clarification. I didn't understand how the 11.x branch worked and couldn't find this item listed in any of the 10.2x release notes so I assumed it wasn't in the 10.2 release. But I've just confirmed it is indeed there.

Thanks @RichardDavies for confirming the fix is in 10.2. I also did not know that the commit to 11.x would be silently ported to 10.x without some commit message being shown here, or some comment from the committer. I'm sure many people knew this, but I didn't, and I always look for commit messages in the comments of the issues. Maybe I should also remember to search in the commit history of the branch.
Viewing https://git.drupalcode.org/project/drupal/-/commits/10.2.x and scrolling down we see

The commit id 5404ebfb is exactly the same as the 11.x commit so I am still unsure wheteher this was a cherry-pick or whether the 11.x branch was actually the 'same' as the 10.2.x branch back then.

It is the new Drupal core branching:
https://www.drupal.org/about/core/blog/new-drupal-core-branching-scheme-... →

Thank you @Martijn de Wit, I knew there was something big going with branch naming, and that post explains it all.

I believe an unintended change was introduced with this issue. If a timestamp field provided a default value or default value callback, that field-level default value is no longer used as the default value in the entity form when creating a new entity. There is a check in TimestampDatetimeWidget::formElement() to only use the field item delta value if the entity is "not new": https://git.drupalcode.org/project/drupal/-/commit/5404ebfb11155b53dee0e...

A timestamp field with a default value might be a common use-case when the field is required (the user must provide a value) but the application would like to preset a default value to the current time for convenience. For an example see 📌 Make timestamp required Fixed

@paul121 nice find, I will try and track down in this issue why that change was made.

Are you able to create a new issue with your bug report and steps to reproduce from a fresh Drupal install?

Originally added in #16 I'm not quite sure why, as you say default values are generally used on new entities so it doesn't make much sense to me. Unfortunate that we didn't pick it up sooner!

Thanks for the *fast* response @acbramley! I've opened a new issue 🐛 Datetime_timestamp widget does not use default field value Active