Add validation constraints to core.date_format.*

Looks good to me. Surprised didn't cause more test failures besides the 1 but woo!

One nitpicky question 😇

Question how often could a new character be added to php date?

That's true guess it will just have to be remembered. Will leave in NR if you want another vote, agree though on the need for tests.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

See there is a test in there now. Should that be expanded though?

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Surprised the bot hasn't got this, but appears to have build failure.

Ran the test-only job and got several test failures (which is good as it shows coverage) https://git.drupalcode.org/issue/drupal-3397491/-/jobs/463228

There is 1 thread open but I believe it has been addressed by @borisson_

Think this is good.

Yeah that's … tricky 😬 Maybe something like For example, use "Y" for the year, "m" for the month, "d" for the day. For more advanced options see .? 🤔

@sourabhjain Please do not ask for a review on every review thread. Also: it seems like you just blindly modified these lines, without running tests locally?

    $this->assertValidationErrors(function () use ($entity_values) {
      DateFormat::create($entity_values)->save();
    });

is nonsense 😳 If you'd run it, you'd see you introduced a PHP fatal error:

"Drupal\KernelTests\Core\Config\ConfigEntityValidationTestBase::assertValidationErrors(): Argument #1 ($expected_messages) must be of type array, Closure given, called in …

This is a contribution that slows progress down. 😔

One usability nit, but other than that this looks great!

(And thanks for already adding FullyValidatable!)

Agreed and fixed.

🚢

#11 is a good question. If PHP 8.4 adds support for a new letter, then we would need to add it here, but that would then potentially allow invalid formats for sites running PHP 8.3.

Then I wondered - do we need a callback somewhere that provides a different string for the regex depending on PHP version? But that seems like overkill.

So what if we just allowed [a-z][A-Z] and it would be a bit looser than reality but we'd never have to think about the above, and it would still catch most completely invalid date formats. Someone can still make a case error with the new validation as long as something else in the string is valid, so it's not completely preventing mistakes.

Interesting. 🤔

This would not catch the following test cases though:

+   * @testWith ["q", true, "This is not a valid date format."]
…
+   *   ["q", false, "This is not a valid date format."]

Alternative idea: what if we made a custom validator that checked if each individual [a-z][A-Z] character actually yielded output? Because characters that are not supported by PHP will just get printed as-is:

<?php
$date=date_create("2013-03-15");
var_dump(date_format($date,"q Y/m/d H:i:s"));
var_dump(date_format($date,"q"));

— https://3v4l.org/PIAV2

#42: I doubt that's ever been intentionally supported? 😅 See the test coverage, for example \Drupal\Tests\system\Functional\Common\FormatDateTest — none of that works that way. Or maybe I'm entirely wrong?

Also … the example you provide is also forbidden by the current MR?

AFAICT that's supposed to be a translatable string, and the translatable string has a placeholder for this formatted date. Putting the string into the formatted date is the wrong way around IMHO.

How often does PHP add new letters to date formats? If the available letters haven't changed in aeons, I'm not sure it's worth making this more complex than the regex we already have in the MR.

#46 is also what I was thinking honestly 😅

Another option here: rather than the regex only listing the characters we allow in a date format, list the characters we forbid. (In other words, just invert the logical outcome of the current regex.) If a new version of PHP comes along that (for example) adds support for the letter Q, we can just remove it from the rejectlist, and older versions of PHP will just harmlessly pass it through.

I also thought #44 was allowed (if not recommended), it's what PHP allows you to do, #48 would explicitly break that behaviour then but if it's not already possible, maybe that is fine?

Oh wow, I didn't know you could escape formatting characters. What a pain.

Okay, in light of that, maybe we should just go back to something simple and relatively naïve, like "forbid control characters". So just reuse type: label.

… which implies "yeah you'll have to check and see how this works". That's probably true already anyway. So … not unreasonable I think.

I don't like it, but considering all factors at play here, it might be the best possible trade-off indeed to just go with type: label + a comment explaining the factors that led to that decision.

OTOH

it's what PHP allows you to do

That should not be the question here. The question should be: How does Drupal intend this DateFormat config entity to be used? And then I think the conclusion would be different, because the validation that type: label gets us is virtually useless 🙈

Wait a minute here. Might we not be massively overcomplicating this?

In the MR, the constraint we have for the date pattern is this:

        Regex:
          pattern: '/[aABcdDeFgGhHiIjlLmMnNoOpPrsStTuUvwWxXyYzZ]/'

In other words: "this string must contain, somewhere, at least one of the characters that we know is valid to use in a date format as of right now".

This regex has no opinion about unsupported characters, or escaping anything, or characters that might be supported in the future. All of those things would sail through it. It merely says you must have at least one of the characters that is supported now.

So, except in the highly unlikely event that PHP 8.4 drops support for all of these characters in date formats...this pattern will continue to be valid. Even if PHP adds support for a new character. And rightfully so - this is already a very permissive validation. Because of that, it's also very future-proof.

Therefore, I'm not sure #11 is looking at this correctly.

I'm restoring RTBC, but if I'm barking up the wrong tree, feel free to knock it back.

EDIT: We could beef up this regex slightly by making it say, in effect, "your string must contain at least one of these characters and it cannot be escaped". Something like: [^\\][aABcdDeFgGhHiIjlLmMnNoOpPrsStTuUvwWxXyYzZ]

Even if PHP adds support for a new character.

The issue would be that someone adds a new date format using only the new character that PHP supports, and then it fails validation.

I don't know whether that will actually happen, but it seems as likely as them adding an unsupported letter by accident that would pass the type: label regex and fail the one here.

My feeling is, if that happened - which already feels edge-casey to me, but who knows - it would be a perfectly legitimate reason to open an issue to add the newly-supported letter to our regex. :) That'd be a one-line fix and probably wouldn't even need test coverage.

And if the functionality was needed right away, one could easily implement hook_config_schema_alter() to change the constraint.

OK I'm convinced. Thanks for talking through it.

Committed/pushed to 11.x, thanks!

Thanks!

(Fixing status, silly d.o and its non-handling of race conditions 😅)