Problem/Motivation

After upgrading from 2.1.0 to 2.1.1 and 2.1.2 I discovered a bug in my custom #states requirement code that asserts empty values.

On further investigation, I found the TimeElement now uses 86401 to represent an empty value, which was added in #3074674: Support empty time range values. Therefore the value of the element is not an empty string anymore, so that makes sense why my #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!

CommentFileSizeAuthor
#8 3463912-MR24-250630.patch12.42 KBtame4tex

Issue fork time_field-3463912

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

tame4tex created an issue. See original summary.

tame4tex’s picture

tame4tex commented
Issue summary: View changes
bramdriesen’s picture

bramdriesen
he/him
Primary language Dutch
Location Belgium 🇧🇪🇪🇺
commented
Parent issue: » #3074674: Support empty time range values

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.

tame4tex’s picture

tame4tex commented

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 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.

bramdriesen’s picture

bramdriesen
he/him
Primary language Dutch
Location Belgium 🇧🇪🇪🇺
commented
Related issues: +#3423941: After 2.1.1 update all time fields are treated as required; empty values not accepted.

Some more background info can be found here: https://www.drupal.org/project/time_field/issues/3423941#comment-15464729

tame4tex opened merge request !24

tame4tex’s picture

tame4tex commented
Issue summary: View changes
Status: Active » Needs review

I have implemented the proposed solution and opened a MR. All tests are passing and it appears to be working well on my site.

Given the complexity of the update hook to update existing table schema and field values I have also removed support for old Drupal versions.

tame4tex’s picture

tame4tex commented
StatusFileSize
new 3463912-MR24-250630.patch12.42 KB

In case anyone else has issues, the patch from the MR needs to be modified to successfully apply via cweagans/composer-patches.

The change to time_field.info.yml needs to be removed. See #3066468: Packaging info from .info.yml often creates conflicts when patching (ddo) for more info.

I have uploaded a working version of the patch.

bramdriesen’s picture

bramdriesen
he/him
Primary language Dutch
Location Belgium 🇧🇪🇪🇺
commented
Issue tags: +Needs manual testing

I need to manually test the upgrade path on an existing site with data. Code wise this looks really good though, so I'm quite confident this will be ok.

And thank you for your work @tame4tex

tame4tex’s picture

tame4tex commented

You're welcome and thanks for being such a great maintainer @bramdriesen!

bramdriesen’s picture

bramdriesen
he/him
Primary language Dutch
Location Belgium 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing

Finally was able to test this manually, this works like it's expected to! Thanks again @tame4tex

bramdriesen’s picture

bramdriesen
he/him
Primary language Dutch
Location Belgium 🇧🇪🇪🇺
commented
Status: Reviewed & tested by the community » Fixed
bramdriesen’s picture

bramdriesen
he/him
Primary language Dutch
Location Belgium 🇧🇪🇪🇺
commented

And released! https://www.drupal.org/project/time_field/releases/2.2.0

Status: Fixed » Closed (fixed)

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

donquixote’s picture

donquixote commented

Since this change, it seems we can no longer set 12:00 AM as a time value in a TimeRangeWidget.

  public function massageFormValues(array $values, array $form, FormStateInterface $form_state) {
    foreach ($values as $delta => $value) {
      if (empty($value['from'])) {
        $values[$delta]['from'] = NULL;
      }
      if (empty($value['to'])) {
        $values[$delta]['to'] = NULL;
      }
    }
    return $values;
  }

This code will replace 0 (12:00 AM) with NULL.
Instead of empty() we should check for empty string.

      if (($value['from'] ?? '') === '') {
        $this->messenger()->addStatus(var_export($value, TRUE));
        $values[$delta]['from'] = NULL;
      }
      if (($value['to'] ?? '') === '') {
        $values[$delta]['to'] = NULL;
      }

Going to open a new issue..

azinck’s picture

azinck commented

The update hook introduced here has major problems that can lead to data loss. I've taken out #3547284: Update hook 10200 can result in data loss as a follow-up.