Problem/Motivation

When using #date_year_range on date form element, it adds min and max HTML attributes that prevents the browser from accepting a date outside this range.
However, if the browser does not support these attributes, the range is never validated server-side.

Steps to reproduce

Create a form with a date element then alter it:

/**
 * Implements hook_field_widget_WIDGET_TYPE_form_alter().
 */
function foo_field_widget_datetime_default_form_alter(array &$element) {
  $element['value']['#date_year_range'] = '1850:3000';
}

Then try to submit a date before 1850 with a browser that does not support the min attribute on date inputs (or with curl).

Proposed resolution

DateTime::validateDatetime() should validate, using the Entity validation API server-side, if the date is in the range.

Issue fork drupal-3269890

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

prudloff created an issue. See original summary.

prudloff opened merge request !1976

prudloff’s picture

prudloff commented
Status: Active » Needs review

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

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should 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.

pieterdc’s picture

pieterdc
Primary language Dutch
Location Ghent
commented

Thanks for sharing, @prudloff!
As far as I've tested it, it works.

Looking at the code, the logic may be located better in a place where it makes $date->hasErrors() (the line above the code changes) fail, but I'm not familiar enough with how these kinds of PHP classes should be coded to claim your code changes need to be reworked, so I'm leaving the issue state as is.

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

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should 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.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative, +Needs tests

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

The MR will need to be updated to 9.5.x or 10.1.x

also will require a test case to show the issue.

Version: 9.5.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. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

prudloff’s picture

prudloff commented
Status: Needs work » Needs review

I rebased and added a test.

Looking at the code, the logic may be located better in a place where it makes $date->hasErrors() (the line above the code changes) fail, but I'm not familiar enough with how these kinds of PHP classes should be coded to claim your code changes need to be reworked, so I'm leaving the issue state as is.

DateTimePlus::hasErrors() checks if the date is a valid date but it has no notion of allowed values.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests 
1) Drupal\KernelTests\Core\Datetime\DatetimeElementFormTest::testRangeValidate
Failed asserting that null matches expected 'The range_datetime_element date is invalid. Date should be in the 1850-3000 year range.'.
/builds/issue/drupal-3269890/core/tests/Drupal/KernelTests/Core/Datetime/DatetimeElementFormTest.php:224
FAILURES!
Tests: 5, Assertions: 11, Failures: 1.

Shows test coverage so removing the tag for tests, also the coverage being added looks great and in depth.

Believe all feedback for this one has been addressed.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

Looks good but has merge conflicts.

shalini_jha made their first commit to this issue’s fork.

shalini_jha’s picture

shalini_jha commented
Status: Needs work » Needs review

I have rebased this MR and resolved the merge conflicts. During the rebase, I noticed that some new behavior was introduced, so to ensure everything continues to work as expected, I re-ran the range test locally.

While debugging, I found that the test was failing in the third scenario where the datetime value falls within the defined range. Due to the newly introduced behavior Here, the form state now includes additional errors such as datetime_required_error and datetime_no_required_error.

To address this, I updated the third scenario’s assertion to only check that range_datetime_element is not present in the form state errors, as this is the specific error we are handling in this case in testRangeValidate() test method. With this change, the test now passes as expected.

Moving this back to Needs Review.Kindly review.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Rebase seems good

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

This is a nice find. I think the MR needs a few adjustments. See the MR review comments.

oily made their first commit to this issue’s fork.

prudloff’s picture

prudloff commented
Status: Needs work » Needs review

I think comments have been addressed.

oily’s picture

oily commented

Added 101 commits from source branch. Re-ran failed functional test. Pipeline is now green.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Was there a merge conflict needed for the rebase?

Feedback has been addressed it seems.

Saving credit for alexpott for the MR reviews.

oily’s picture

oily commented

Corrected code comment at line 396. We are validating the date, not 'checking' it. Similar idea but let's be consistent and specific!

Checking is something we do a lot in PHP generally eg using function like is_null() or by using if statements. On the other hand validation is specific to forms and form entry and the validation component

https://symfony.com/doc/6.4/validation.html

oily’s picture

oily commented
Status: Reviewed & tested by the community » Needs review
smustgrave’s picture

smustgrave commented

Personally don’t see why the comment change was needed. Read fine before

smustgrave’s picture

smustgrave commented
Issue tags: -Needs Review Queue Initiative
oily’s picture

oily commented

RE: #23 @smustgrave It could help if someone greps the codebase? They'd return i think a lot more with 'check' than 'validate'. Also the IS distinguishes between the 'checking' done client-side by the browser to the entity validation api validation done 'server-side'. We want the field value to be validated as opposed to just 'checked'? Also 'checking' can create muddle as we also 'check' checkboxes in forms api.

In fact, browser-side 'checking' is also 'validation'.

https://developer.mozilla.org/en-US/docs/Learn_web_development/Extension...

Client-side validation is an initial check and an important feature of good user experience; by catching invalid data on the client-side,..

So describing the browser-side an 'initial check' or a 'check' prior to the full-on 'validation' server-side still seems like a useful semantic distinction to preserve. Generally it seems an important distinction because client-side 'validation' is always unsafe and can easily be bypassed whereas if used correctly, server-side is secure.

It seems useful to me to know I can grep the codebase or a portion of it for 'validation' and see what role is played by the entity valdiation api in Drupal..

oily’s picture

oily commented
Issue summary: View changes
oily’s picture

oily commented

Updated IS according to #21 and #24.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Restoring previous status.

quietone’s picture

quietone commented

This is passing the core gates and I updated credit. I read the MR and didn't see anything amiss.

  • alexpott committed 6c481f74 on 11.2.x 
    Issue #3269890 by prudloff, smustgrave, catch, shalini_jha, alexpott,...

  • alexpott committed 2558bc86 on 11.x 
    Issue #3269890 by prudloff, smustgrave, catch, shalini_jha, alexpott,...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: 11.x-dev » 11.2.x-dev
Status: Reviewed & tested by the community » Fixed

Committed 2558bc8 and pushed to 11.x. Thanks!
Committed 6c481f7 and pushed to 11.2.x. Thanks!

This didn't cherry pick clealy to 10.6.x / 10.5.x so I did not backport there.

Now that this issue is closed, please review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, please credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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

prudloff closed merge request !1976

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Related issues: +#2942828: Remove hard-coded #date_year_range making it impossible to enter dates < 1900, > 2050. Instead make them optional and configurable in the UI

This bugfix caused some kind of "regression", because before this fix (from users perspective) you were able to enter a date like "2099".
Now with this fixed, these nodes can't be saved any more, as the hard coded limit is used and limits to 2037 or 2050!

See #2942828: Remove hard-coded #date_year_range making it impossible to enter dates < 1900, > 2050. Instead make them optional and configurable in the UI. I've been wondering why I suddenly ran into these cases after the last core update. Not being able to enter such future dates is a major issue in my eyes? Even worse it's not configurable: #2836054: Allow configuring of datelist year range date_year_range via UI

Should this get reverted until the other issues are fixed or how can we put a focus on that? I assume we'll see not just a few affected projects!