Problem/Motivation

Currently \Drupal\datetime_range\Plugin\Field\FieldWidget\DateRangeWidgetBase::validateStartEnd() is validating date ranges - this means that the validation is in the form layer and not the data layer.

Proposed resolution

Create a constraint to handle this validation.

Remaining tasks

Add unit test for constraint.

User interface changes

None

API changes

An additional constraint for something that is currently only enforced by a form.

Data model changes

None

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

Status: Active » Needs review
FileSize
4.84 KB

Here's a first cut of the change.

alexpott’s picture

alexpott’s picture

If you have a date range with times and you don't complete the time this validator produces the following error...

Warning: DateTime::createFromFormat() expects parameter 2 to be string, array given in Drupal\Component\Datetime\DateTimePlus::createFromFormat() (line 220 of core/lib/Drupal/Component/Datetime/DateTimePlus.php).
Drupal\Component\Datetime\DateTimePlus::createFromFormat('Y-m-d\TH:i:s', Array, 'UTC') (Line: 51)

Status: Needs review » Needs work

The last submitted patch, 2: 2847041-2.patch, failed testing.

mpdonadio’s picture

#4, I have to run this locally tonight to see exactly what is happening, but the fails look like they are coming from DateRangeFieldTest::testDatelistWidget(), which is the select version of the widget. So, the time coming out of the form is an array or time parts. I think I ran into this in #2824717: Add a format constraint to DateTimeItem to provide REST error message, but that issue fell off my radar. Maybe, we can type check in the constraint and implode($value, ':') ?

mpdonadio’s picture

#4, the problem is that the constraint is running on the element value from the datelist, which is an array at that point. Not sure how we should handle it.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mpdonadio’s picture

Title: Move Date range validation into a constraint » Add a format and start/end constraints to DateRangeItem to provide REST error message
Issue tags: +API-First Initiative
Related issues: +#2824717: Add a format constraint to DateTimeItem to provide REST error message
mpdonadio’s picture

Assigned: Unassigned » mpdonadio
Status: Needs work » Needs review
Issue tags: +Needs tests
FileSize
11.38 KB

Redo; work in progress. Still need to do the tests.

Heavily copied^H^H^H^H^H^Hbased on #2824717: Add a format constraint to DateTimeItem to provide REST error message.

Status: Needs review » Needs work

The last submitted patch, 11: 2847041-11.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
16.68 KB
900 bytes

This should fix the fail in #11; passes locally now.

Still need to do the EntityTestResourceTestBase tests, but would love feedback on the current patch, especially any thoughts on reusing parts of the Datetime constraints. Seems like something should, but everything seems just different enough (mainly the messages) to prevent reuse.

mpdonadio’s picture

FileSize
11.39 KB
900 bytes

Forgot to rebase everything.

mpdonadio’s picture

Work in progress...

mpdonadio’s picture

+++ b/core/modules/datetime_range/src/Functional/EntityResource/EntityTest/EntityTestDatetimeTest.php
@@ -0,0 +1,209 @@
+      // Errors?
+      $value = ['2017', '03', '01', '21', '53', '00'];
+      $end_value = static::$endDateString;
+      $message = "Unprocessable Entity: validation failed.\n{$fieldName}.0: The daterange start value must be a string.\n{$fieldName}.0: The daterange end value must be a string.\n{$fieldName}.0.value: This value should be of the correct primitive type.\n{$fieldName}.0.end_value: This value should be of the correct primitive type.\n";
+      $this->doEdgeCaseCall($method, $url, $request_options, $value, $end_value, $message);
+
+      // Works!
+      $value = static::$startDateString;
+      $end_value = ['2017', '03', '02', '21', '53', '00'];
+      $message = "Unprocessable Entity: validation failed.\n{$fieldName}.0: The daterange end value must be a string.\n{$fieldName}.0.end_value: This value should be of the correct primitive type.\n";
+      $this->doEdgeCaseCall($method, $url, $request_options, $value, $end_value, $message);
+
+      // Works??
+      $value = ['2017', '03', '01', '21', '53', '00'];
+      $end_value = ['2017', '03', '02', '21', '53', '00'];
+      $message = "Unprocessable Entity: validation failed.\n{$fieldName}.0: The daterange start value must be a string.\n{$fieldName}.0: The daterange end value must be a string.\n{$fieldName}.0.value: This value should be of the correct primitive type.\n{$fieldName}.0.end_value: This value should be of the correct primitive type.\n";
+      $this->doEdgeCaseCall($method, $url, $request_options, $value, $end_value, $message);
+

This has me baffled. The first doEdgeCaseCall() call errors out. If I comment that out, the next two work. I cannot trace out what is wrong here.

mpdonadio’s picture

Put tests in right place... Still working on this, but ideas on the errors appreciated.

Status: Needs review » Needs work

The last submitted patch, 17: 2847041-17.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

jhedstrom’s picture

  1. +++ b/core/modules/datetime_range/src/Plugin/Validation/Constraint/DateRangeFormatConstraint.php
    @@ -0,0 +1,59 @@
    + *   label = @Translation("Daterange formats valid for Daterange type.", context = "Validation"),
    ...
    +  public $badStartType = "The daterange start value must be a string.";
    ...
    +  public $badEndType = "The daterange end value must be a string.";
    

    Since this is user-facing text, let's use "date range" since I don't think daterange is actually a word.

  2. +++ b/core/modules/datetime_range/tests/src/Functional/EntityResource/EntityTest/EntityTestAlldayTest.php
    @@ -0,0 +1,21 @@
    +namespace Drupal\Tests\datetime_range\Functional\EntityResource\EntityTest;
    

    The new tests are all functional. Can the new validator be unit tested? Quite a few of our custom validators do have unit tests.

Regarding the erroring out, I think it has something to do with these validations still running on an array instead of a string:

    /* @var $item \Drupal\datetime_range\Plugin\Field\FieldType\DateRangeItem */
    if (isset($item)) {
      $value = $item->getValue()['value'];
      $end_value = $item->getValue()['end_value'];

      if ($end_value < $value) {
        $this->context->addViolation($constraint->badStartEnd, [
          '@value' => $value,
          '@end_value' => $end_value,
        ]);
      }
    }

I'm not sure though why sometimes it doesn't throw the error even when one of the values is an array. The actual warning that causes the error happens when this violation gets formatted and @value is expected to be a string.

mpdonadio’s picture

Status: Needs work » Needs review

Re #19-2, in the sibling issue for datetime fields, @WimLeers wanted EntityTestResourceTestBase tests, I think so it would cover end-to-end testing in preparation for the (de)normalizer (which is next up on my plate).

If this looks mostly OK, I will refactor EntityTestDaterangeTest into a test base for the common bits, and make versions for date-only and all-day.

Re the error, the validators all run and can't(?) be cancelled if one fails, so the start/end one was running on the bad inputs, which isn't a valid test. See also the is_string() in DateRangeFormatConstraintValidator. That is from a weird case where an array can be passed in when using the datelist widget (one of the comments in #2824717: Add a format constraint to DateTimeItem to provide REST error message has the details).

mpdonadio’s picture

Status: Needs review » Needs work

The last submitted patch, 21: 2847041-20.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

mpdonadio’s picture

Made some progress with this. This should come up green, but I noticed a bigger problem with the 'allday' variant.

'allday' is supposed to be midnight-to-midnight in the time zone of entry. The widget uses date-only form elements, but then takes the local times 00:00:00 and 23:59:59, converts to UTC, and sticks this in storage.

I'm not really sure how to enforce this from a REST perspective (or even just at the field API level; see also #2716891: DateTimeIso8601::setDateTime() vs. DateTimeItem::schema(): (at least) one of them is broken).

Note that even if we fix the ISO8601 problem, we still have the difference between a time zone and an offset from UTC. 'Australia/Sydney' is a time zone; '+11:00' is an offset from UTC that can be in one or more time zones with different DST rules / history.

Ideas?

Wim Leers’s picture

  1. but then takes the local times 00:00:00 and 23:59:59, […]

    I don't see this in the tests yet. A failing test would help to understand this.

  2. Note that even if we fix the ISO8601 problem, we still have the difference between a time zone and an offset from UTC. 'Australia/Sydney' is a time zone; '+11:00' is an offset from UTC that can be in one or more time zones with different DST rules / history.

    Why is this a problem new to date range fields? Why isn't this a problem in HEAD already? I can't imagine this wasn't discussed before?

  3. +++ b/core/modules/datetime_range/src/Plugin/Validation/Constraint/DateRangeFormatConstraint.php
    @@ -0,0 +1,59 @@
    +class DateRangeFormatConstraint extends Constraint {
    
    +++ b/core/modules/datetime_range/src/Plugin/Validation/Constraint/DateRangeFormatConstraintValidator.php
    @@ -0,0 +1,96 @@
    +class DateRangeFormatConstraintValidator extends ConstraintValidator {
    

    All of these are constraints that apply to \Drupal\Core\TypedData\Plugin\DataType\DateTimeIso8601.

    Related: #2926508-24: Add DateTimeNormalizer+TimestampNormalizer, deprecate TimestampItemNormalizer: @DataType-level normalizers are reusable by JSON:API:

    It looks like all fields using the DateTimeIso8601 @DataType are happy to allow RFC3339 timestamps, (for example 2017-03-01T20:02:00-00:00, the -00:00 at the end is allowed by RFC3339 but not by ISO8601), it's only the DB layer that's preventing this from happening. IOW: \Drupal\datetime\Plugin\Validation\Constraint\DateTimeFormatConstraintValidator::validate() is not strict enough yet.

    (The same is true for \Drupal\datetime\Plugin\Validation\Constraint\DateTimeFormatConstraint.)

  4. +++ b/core/modules/datetime_range/src/Plugin/Validation/Constraint/DateRangeStartEndConstraintValidator.php
    @@ -0,0 +1,38 @@
    +      // We cannot get the computed values from the item at this point, so we
    +      // have to use string comparison. This may give false error messages, but
    ...
    +        if ($value > $end_value) {
    

    How can this work at all?! :O

  5. +++ b/core/modules/datetime_range/tests/src/Functional/EntityResource/EntityTest/EntityTestAlldayTest.php
    @@ -0,0 +1,101 @@
    +class EntityTestAlldayTest extends EntityTestDaterangeTestBase {
    

    Interesting, using REST integration tests instead of validation-focused kernel tests!

    I think that's okay.

Wim Leers’s picture

Also: wow, this is impressively thorough already! 👏

borisson_’s picture

Status: Needs review » Needs work

I don't have an answer for the questions in #23, but I have some nitpicks + one actual error (I think).

I didn't look at the tests, shouldn't we also write unit/kernel test coverage for the constraints?

  1. +++ b/core/modules/datetime_range/src/Plugin/Validation/Constraint/DateRangeFormatConstraintValidator.php
    @@ -0,0 +1,96 @@
    +    /* @var $item \Drupal\datetime_range\Plugin\Field\FieldType\DateRangeItem */
    +    if (isset($item)) {
    

    We can make this entire thing indented less deep by switching this if around.

    if (!isset($item)) {
      return
    }
    
    ... // rest goes here.
    
  2. +++ b/core/modules/datetime_range/src/Plugin/Validation/Constraint/DateRangeFormatConstraintValidator.php
    @@ -0,0 +1,96 @@
    +      if ($stop) return;
    

    Can we make this into a proper statement?

    if ($stop) {
    return;}
    
  3. +++ b/core/modules/datetime_range/src/Plugin/Validation/Constraint/DateRangeFormatConstraintValidator.php
    @@ -0,0 +1,96 @@
    +      $format = $datetime_type === DateRangeItem::DATETIME_TYPE_DATE ? DateTimeItemInterface::DATE_STORAGE_FORMAT : DateTimeItemInterface::DATETIME_STORAGE_FORMAT;
    

    Can we changes this from a ternary to a proper if? I had to read this 3 times because it looked like it said the same thing, I think we'd benefit from having those lines under each other because the different lenghts would give it away easily.

  4. +++ b/core/modules/datetime_range/src/Plugin/Validation/Constraint/DateRangeFormatConstraintValidator.php
    @@ -0,0 +1,96 @@
    +      if ($stop) return;
    

    Same remark as earlier.

  5. +++ b/core/modules/datetime_range/src/Plugin/Validation/Constraint/DateRangeStartEndConstraintValidator.php
    @@ -0,0 +1,38 @@
    +    /* @var $item \Drupal\datetime_range\Plugin\Field\FieldType\DateRangeItem */
    +    if (isset($item)) {
    

    We can invert the if to reduce indentation. The comment here would need to reflowed as well.

I started reviewing this before Wim came along, so sorry if some of these are double. But I agree with #25, impressive work!

Wim Leers’s picture

What a coincidence that we'd be reviewing the same patch simultaneously :D

mpdonadio’s picture

Assigned: Unassigned » mpdonadio
Status: Needs work » Needs review
FileSize
31 KB
7.74 KB

Addressed some of the feedback.

#24-1,2: Struggling a little bit here since I can't seem to express this as a test. If you look at DateRangeFieldTest::testAlldayRangeField(), you will see what we test. We collect just the date and no time, but the time portion that goes into storage as midnight (warped to UTC), this ends up as a range from 00:00:00 to 23:59:59 in the user's time zone. This is what differentiates is from date-only. The analogy is Christmas this year is 2017/12/25 on the calendar (date-only), but Christmas for me (allday) in America/New_York is 2017-12-25T00:00:00-05:00 to 2017-12-25T23:59:59-05:00 (or 2017-12-25T00:05:00+00:00 to 2017-12-26T04:59:59+00:00 in UTC). I supposed in the constraint I can take the values (which are supposed to be UTC at that point), and convert to the user's time zone and make sure they are midnight?

#24-3,5: we did the datetime constraint on the field w/ REST tests (that issue was more geared towards better DX with the error messages, though) which is what I mirrored here. Looking at this closer, we probably need more coverage in DateRangeItemTest to parallel what is done in DateTimeItemTest, too.

#24-4: not sure what you mean by "How can this work at all", since Drupal\Core\TypedData\Plugin\DataType\Map::getValue() is

  public function getValue() {
    // Update the values and return them.
    foreach ($this->properties as $name => $property) {
      $definition = $property->getDataDefinition();
      if (!$definition->isComputed()) {
        $value = $property->getValue();
        // Only write NULL values if the whole map is not NULL.
        if (isset($this->values) || isset($value)) {
          $this->values[$name] = $value;
        }
      }
    }
    return $this->values;
  }

which explicitly excludes computed values.

(I already rolled this patch and have been working on this comment for ~45min now, but I guess I can just use $item->start_date and $item->end_date directly to get the computed values and not use $item->getValue(); that is what we just do in the formatters...).

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Wim Leers’s picture

Wim Leers’s picture

Issue tags: +Entity Field API

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Wim Leers’s picture

Wim Leers’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/datetime_range/src/Plugin/Validation/Constraint/DateRangeFormatConstraint.php
    @@ -0,0 +1,59 @@
    +  public $badStartType = "The date range start value must be a string.";
    ...
    +  public $badStartFormat = "The date range start value '@value' is invalid for the format '@format'";
    

    I'd expect these to exist on \Drupal\Core\TypedData\Plugin\DataType\DateTimeIso8601, not on \Drupal\datetime_range\Plugin\Field\FieldType\DateRangeItem.

    Note that as of #2926508: Add DateTimeNormalizer+TimestampNormalizer, deprecate TimestampItemNormalizer: @DataType-level normalizers are reusable by JSON:API, the denormalizers already prevent invalid values from getting set for at least REST and JSON:API :)

  2. +++ b/core/modules/datetime_range/tests/src/Functional/EntityResource/EntityTest/EntityTestDaterangeTest.php
    @@ -0,0 +1,101 @@
    +class EntityTestDaterangeTest extends EntityTestDaterangeTestBase {
    

    This test was effectively already added in #2926508: Add DateTimeNormalizer+TimestampNormalizer, deprecate TimestampItemNormalizer: @DataType-level normalizers are reusable by JSON:API — see \Drupal\Tests\datetime_range\Functional\EntityResource\EntityTest\EntityTestDateRangeTest. I'm not sure we need to add detailed functional tests for each case, since the normalization is now happening at the @DataType level rather than the @FieldType level!

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Wim Leers’s picture

Title: Add a format and start/end constraints to DateRangeItem to provide REST error message » Add a format and start/end validation constraints to DateRangeItem to provide helpful REST/JSON:API error message

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

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

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

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

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

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.

jibla’s picture

Why is not this patch merged into core yet?

I see the tests in the patch, however issue status still says it needs unit tests.

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.

jaime@gingerrobot.com’s picture

Issue tags: +field validation
jaime@gingerrobot.com’s picture

Status: Needs work » Needs review

Setting to needs review to see what is left to be done especially in regards to tests as many were added in other tickets.

needs-review-queue-bot’s picture

Status: Needs review » Needs work
FileSize
13.6 KB

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or 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.

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.