backdrop / backdrop-issues

Issue tracker for Backdrop core.
144 stars 40 forks source link

Fix fatal error when submitting a form that has a hidden date field and uses the HTML5 Date widget #6027

Closed argiepiano closed 1 year ago

argiepiano commented 1 year ago

Description of the bug

When you submit a form (e.g. the "Add user account form") with a hidden Date field that provides a default value (e.g. "Now") and uses the HTML5 widget, you get a fatal error (PHP 8.1) or a warning (>PHP 7.4) TypeError: DateTime::createFromFormat(): Argument #2 ($datetime) must be of type string, array given in DateTime::createFromFormat().

This was first reported in the forum.

Steps To Reproduce

  1. Add a Date field to the user account entity at admin/config/people/manage. Be sure to select the HTML5 widget. In Date granularity, uncheck "hours", "minutes" and "seconds", so that the widget only accepts year, month and day.
  2. Be sure to deselect "Display on user registration form." This will hide the field in the user registration form
  3. Choose "Now" as default value
  4. Save
  5. Go to the Add user account at admin/people/create
  6. Verify that the date field you added above is NOT showing
  7. Enter the information and save the form

The warning or fatal error will appear.

Actual behavior

The form fails to submit, or even worse, you get a fatal error.

Expected behavior

The user should be created

Additional information

It took quite a bit of digging, but this is what's happening.

So, a few questions:

  1. Should the element validation function still be called when the field is hidden? I don't know if this is "normal behavior".
  2. If it is, there are two ways in which we could solve this a. Either add a #value_callback that will take care of transforming the array into a string, b. OR, harden the validation callbacks html_date_validate() etc to anticipate the presence of an array, and act accordingly

Thoughts?

BTW, this is also happening with other forms, e.g. node_form, if you alter the form and make a field #access => FALSE.


A related error is thrown by html_time_validate() under similar conditions (hidden form field). To reproduce this one you must use the html_time FAPI element (can't reproduce with a Date field). I'm also attaching a simple module that shows both errors with hidden FAPI elements html_date and html_time.

  1. Create a form in a custom module with these elements:
  $form['html_time'] = array(
    '#title' => 'html_time',
    '#type' => 'html_time',
    '#default_value' => array(
      'time' => '20:30'
    ),
   '#access' => FALSE,
  );

  $form['submit'] = array(
    '#type' = > 'submit',
    '#value' => 'Submit',
  );
  1. Visit the form and submit.

The following error is thrown:

TypeError: preg_match(): Argument #2 ($subject) must be of type string, array given in preg_match() (line 3338 of /Users/XXX/Sites/localhost/panettone/core/includes/form.inc).


Attached module to reproduce errors (uses FAPI html_date and html_time):

Simply install and visit the following: /test-fapi-html-date /test-fapi-html-time

testing_fapi.zip


PR: https://github.com/backdrop/backdrop/pull/4384

bugfolder commented 1 year ago

Should the element validation function still be called when the field is hidden? I don't know if this is "normal behavior".

Seems like telling the user that there's an error in something that the user can't see or do anything about is pretty mean, so "No" makes sense to me.

onyxnz commented 1 year ago

I'm in favour of hardening the validation callbacks to anticipate the array, as the form.inc currently makes assumptions about what it's receiving, and can that be exploited? But as @bugfolder said, the whole submission of invisble data thing is concerning.

argiepiano commented 1 year ago

Should the element validation function still be called when the field is hidden? I don't know if this is "normal behavior".

Seems like telling the user that there's an error in something that the user can't see or do anything about is pretty mean, so "No" makes sense to me.

Actually, I checked with other elements. Element validation is "normal behavior" even for hidden fields or FAPI elements (i.e. #access => FALSE) that have an #element_validate property. Since the HTML5 widget form uses the FAPI element html_date, and since this element has '#element_validate' => array('html_date_validate'),, it will call this validation regardless of whether the user has access to the field or not.

Typically, when fields are "hidden", the #default_value becomes the submitted value (but is not passed to the submit handler). Normally, the #default_value is the same type as the submitted value (e.g. a string), except in this case, where there is a mismatch between the two.

argiepiano commented 1 year ago

Wondering if @indigoxela has any thoughts about how to approach a fix to this issue, since you did much work on getting the HTNML5 widget going?

indigoxela commented 1 year ago

I'm having trouble to reproduce.

Following the steps on PHP 8.1, I can still save the new user without problems. Which step is missing?

I tried both, creating as admin and registering as new user, both worked without any warnings or errors.

argiepiano commented 1 year ago

@indigoxela, thank you for checking on this. I made a mistake in the steps above. The error happens only for dates without hours, minutes or seconds. I have fixed the directions above.

argiepiano commented 1 year ago

There is also a related error, thrown by html_time_validate() under similar conditions. To reproduce this one you must use the html_time FAPI element (can't reproduce with a Date field).

Since they are related (and both will use a similar fix), I'm adding steps on how to reproduce in the OP.

argiepiano commented 1 year ago

I have also attached a small module that shows the errors produced by hidden html_date and html_time upon element validation. Attached to the OP.

argiepiano commented 1 year ago

PR submitted and ready for review and testing.

I decided to create a #value_callback for html_date and html_time. This callback takes care of extracting the value from the #default_value array, and returning a string when there is no input (e.g. when submitting a form that has a hidden element of this type). This avoids the errors in the element validation functions.

indigoxela commented 1 year ago

The error happens only for dates without hours, minutes or seconds

@argiepiano ah, now I get it. Yes, the Date module field allows odd settings, but the html_datetime FAPI element does require a full datetime (the html_date element does not). Seconds are optional, though, if IRC.

A #value_callback might be the solution for that, not sure if it's the best one.

FTR core ships with three different FAPI elements, html_date, html_time and html_datetime. It might lead to strange timezone problems, if we just use the html_datetime element for date-only fields, not sure.

argiepiano commented 1 year ago

FTR core ships with three different FAPI elements, html_date, html_time and html_datetime. It might lead to strange timezone problems, if we just use the html_datetime element for date-only fields, not sure.

@indigoxela, thanks for the response! To clarify, the Date field in the OP that uses a HTML5 widget DOES use FAPI html_date (not html_datetime) when you deselect hours., minutes and seconds in the field setting. So, what you mention above is not the problem. Rather, the problem is that the element validation functions for both html_date and html_time expect to receive a string in $element['#value'], but instead receive an array (the one stored in #default_value) when the field is hidden in the form with '#access' => FALSE. The PR I submitted takes care of that through a value callback.

If you have the time, it would be great if you could take a closer look at the PR. 🙏🏽

indigoxela commented 1 year ago

@argiepiano well, forget my previous comment - it was based on wrong assumptions and not on actual testing.

I took the time for actual testing now, and can confirm that the PR fixes the problem. :+1:

argiepiano commented 1 year ago

I appreciate it, @indigoxela! We miss you around here as of late! 😄

argiepiano commented 1 year ago

Any chance to include this fix in the next release? It has a WFM, but it needs a code review.

avpaderno commented 1 year ago

Should not html_date_or_time_value_callback() have also a @ㅤsince tag?

herbdool commented 1 year ago

@kiamlaluno my understanding is that the @since is for API or functions that can be used elsewhere. So not needed here in my opinion.

herbdool commented 1 year ago

@argiepiano the fix looks good to me. I would mark it ready except that I think it needs a test. One for each field type.

argiepiano commented 1 year ago

@herbdool, I've added the test you requested. Can you please take a new look at this bug fix?

The phpcs error and 8.1 error seem unrelated to the modified files.

herbdool commented 1 year ago

@argiepiano thanks for the tests; they look like they do the job. I'll rerun the failing tests and see if that changes anything.

herbdool commented 1 year ago

The one test passed now, but the phpcs didn't run at all. I think it's because your branch is a few commits behind backdrop/1.x. If you bring it up to date it may all pass.

argiepiano commented 1 year ago

The one test passed now, but the phpcs didn't run at all. I think it's because your branch is a few commits behind backdrop/1.x. If you bring it up to date it may all pass.

Coding standards passed. Spelling (fastcgi - unrelated to this pr) did not. Functional tests passed. Is this ready?

herbdool commented 1 year ago

Agreed @argiepiano. Somewhere we can add "fastcgi" as an acceptable spelling but it doesn't need to be in this PR.

quicksketch commented 1 year ago

@argiepiano Could you confirm a similar problem does not exist when using a #type = html_datetime element? From the description of this issue and the code, it looks like datetime elements would have the same potential problem, since they also take an array default value but the browser-submitted value is a string (I think)?

I posted some feedback to the PR. The approach looks good to me but I'd appreciate if we consistently used "disabled", "hidden", and "access" terminology, since they all mean something different. See https://github.com/backdrop/backdrop/pull/4384#pullrequestreview-1465967495

argiepiano commented 1 year ago

Thanks, @quicksketch.

I can confirm that this problem does not exist for element html_datetime. Only for html_date and html_time. In the case of html_datetime elements, the function html_datetime_validate() takes care of breaking down the returned value from the form, and running the date and/or time validations with the correct type of arguments (strings).

I'll check the feedback next.

argiepiano commented 1 year ago

@quicksketch I've pushed your requested changes. The test now tests for both, access FALSE and disabled TRUE.

quicksketch commented 1 year ago

Thanks @argiepiano for the naming changes and the additional tests on disabled elements. I have merged https://github.com/backdrop/backdrop/pull/4384 into 1.x and 1.25.x. Thanks!

https://github.com/backdrop/backdrop/commit/30c33053fe411918cbbd5b5125376a80f16697be by @argiepiano, @bugfolder, @onyxnz, @indigoxela, @kiamlaluno, and @herbdool.