backdrop / backdrop-issues

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

PHP compatibility: Future-proof date formatters and their tests #6310

Open klonos opened 10 months ago

klonos commented 10 months ago

This came up while I was working on #6308, where I was getting so odd discrepancies in the output of date_format_date() between various versions of PHP. Some of these could be explained because of known bugs that are plaguing minor versions of PHP. For example, see the output of https://3v4l.org/tvFJm and notice the 10min difference in the following PHP versions:

Some other discrepancies I could not explain until I noticed the following things (for which I am planning to file a PR to fix):

  1. In DateAPITestCase::testDateAPI(), we have a hard-coded (and silly) array of date formatters. What may initially seem as a full list of lower- and upper-case English letter a-z and A-Z is in fact a reduced list, with some letters omitted from that list (although no apparent reason nor anything stated in any inline comment). I am assuming that that was in order to mimic the list of available formatters in https://www.php.net/manual/en/datetime.format.php. However, as stated at the bottom of that list:

    Unrecognized characters in the format string will be printed as-is."

    So I don't know why we we limiting the letters we are checking to only a specific set that might have been available at the time the test was written but was expanded in the future - we should be checking the entire English alphabet, both lower- as well as upper-case. For any letters that are not valid PHP date formatters (not yet - see point 3 of this list later), there would automatically be a "passthrough" effect by the code we already have in place (because we are using native PHP functions at the end of the day, with some modifications for specific cases - see the switch () in our date_format_date() function) ...and if that was not the case (unrecognized characters not being printed as-is), then we'd have ourselves a bug.

  2. Then, still in DateAPITestCase::testDateAPI(), we have two variables, one using our custom date_format_date() - the other using PHP's native date_format(). Then we are comparing the output of those two variables against each other, expecting them to be the same (using assertEqual()). However, we are using date_now() as the 1st parameter for those two individually. The side effect of doing that is some micro-discrepancies when checking things like microseconds and milliseconds. So what we should be doing instead is to use a 3rd variable and set its value to date_now(), and then use that variable (which will have the same value) as the 1st parameter for both variables we were comparing. That ensures that they are both using the exact same time, down to the millisecond.
  3. Because we are using that hard-coded list of letters to check for date formatters, we are missing out on any new additions in future versions of PHP, and that might have unpredicted side effects. At this point, I should point out that this should only be a display issue - not a data issue, because as stated in the docblock of date_format_date():

    ... Should only be used for display, not input, because it can't be parsed.

    So, in PHP 7.4, I was getting this error:

    • The "v" formatter was expected to be formatted as "830", but was formatted as "v" by Backdrop's date_format_date().

    In PHP 8.2, I was getting these errors:

    • The "p" formatter was expected to be formatted as "Z", but was formatted as "p" by Backdrop's date_format_date().
    • The "v" formatter was expected to be formatted as "818", but was formatted as "v" by Backdrop's date_format_date().
    • The "x" formatter was expected to be formatted as "2023", but was formatted as "x" by Backdrop's date_format_date().
    • The "X" formatter was expected to be formatted as "+2023", but was formatted as "X" by Backdrop's date_format_date().
    Then of course, when checking https://www.php.net/manual/en/datetime.format.php I saw things like: format character Description Example returned values

    ... X | An expanded full numeric representation of a year, at least 4 digits, with - for years BCE, and + for years CE. | Examples: -0055, +0787, +1999, +10191 x | An expanded full numeric representation if requried, or a standard full numeral representation if possible (like Y). At least four digits. Years BCE are prefixed with a -. Years beyond (and including) 10000 are prefixed by a +. | Examples: -0055, 0787, 1999, +10191 ... p | The same as P, but returns Z instead of +00:00 (available as of PHP 8.0.0) | Examples: Z or +02:00 ...

    All these things we missed, because (for whatever reason) we decided to not be checking and accounting for all possible letters.

Lets try to fix all those things and prevent weirdness across different versions of PHP. I am not suggesting to necessarily provide functionality that polyfills formatters consistently across PHP versions (although we could) - just to have our tests check all possible letters so that we can catch these differences early (and then decide what to do about it).

Anyway, I'll file a PR of the changes I am suggesting, and lets discuss further...

klonos commented 10 months ago

PR up for review: https://github.com/backdrop/backdrop/pull/4586 (only the LayoutUpgradePathTest gremlins are failing)

klonos commented 10 months ago

...and to analyze what this change means when fixing vs. ignoring this problem:

So 👎🏼 👎🏼 to ignore vs. 👎🏼 👍🏼 to fix.

We also need to factor in: