Contrib.social

The integer 86401 should not be used to represent an "empty" time element

Open on Drupal.org →
Created on 26 July 2024, 4 months ago

Problem/Motivation

After upgrading from 2.1.0 to 2.1.1 and 2.1.2 I discovered that using a time element in a remote #states value='' condition no longer works.
For example:

  $form['field_test_string']['widget'][0]['value']['#states'] = [
    'invisible' => [
      ':input[name="field_test_time[0][value]"]' => ['value' => ''],
    ],
  ];

On further investigation, I found the `TimeElement` now uses `86401` to represent an empty value, which was added in ✨ Support empty time range values Fixed . Therefore the value of the element is not an empty string anymore, so that makes sense why the #states code doesn't work.

I would argue using `86401` to represent empty is not the correct approach to this problem. It is an invalid value for this input type, leading to a browser console warning and could cause unexpected "gotchas".

I understand its "invalidness" makes it work, but I think there might be a better solution. I also understand and respect the original author of the solution knew it wasn't the best approach.

Proposed resolution

As an alternative solution couldn't we:

  1. Change the `TimeRangeType` schema to allow `to` to be `NULL` (this would also require an update hook)
  2. Implement a `\Drupal\time_field\Plugin\Field\FieldWidget\TimeRangeWidget::massageFormValues()` and set the empty `to` value to NULL to ensure it passes validation.
  3. Modify TimeRangeFormatter to handle a NULL `to` value.

Given I don't use the TimeRangeType widget I have only done preliminary testing of this solution, but from what I have done it seems to work. That said, I could be completely missing something!

🐛 Bug report
Status

Active

Version

2.0

Component

Code

Created by

🇨🇦Canada tame4tex

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

  • Issue created by @tame4tex
  • Comment 4 months ago →
    🇨🇦Canada tame4tex
  • Comment 4 months ago →
    🇧🇪Belgium BramDriesen Belgium 🇧🇪

    I vaguely remember trying NULL in the original issue as that also seemed more appropriate for me. But for some reason that was not possible (don't remember exactly why, guess if I look a bit at the code I might remember).

    Test coverage for this use case is pretty OK now, so we can try to change this to NULL.

    Probably good to add a test for states as well.

  • Comment about 2 months ago →
    🇨🇦Canada tame4tex

    So I finally had some time to revisit this issue and add states testing.

    I have added the states testing in a separate issue https://www.drupal.org/project/time_field/issues/3479769 📌 Add form conditional states testing Active to keep this issue as simple as possible.

    It turns out my initial assumption was incorrect, states is actually working. It was our custom code that interacts with states that was failing due to expecting an empty string would mean empty.

    Regardless, I am still going to work on the proposed resolution to see if NULL will work. In my opinion it is a much better approach than relying on 86401, if for no other reason than to get rid of the console warnings.

  • Comment about 1 month ago →
    🇧🇪Belgium BramDriesen Belgium 🇧🇪

    Some more background info can be found here: https://www.drupal.org/project/time_field/issues/3423941#comment-15464729 🐛 After 2.1.1 update all time fields are treated as required; empty values not accepted. Postponed: needs info

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024