Add validation constraints to core.date_format.*

I'm not sure how to add validation to the format, because it looks like all different kinds of things are allowed, so I've added not null, so we at least have something to validate against.
Passing null into DateTime::format is also not allowed, so it also helps that case.

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

One nitpicky question 😇

Setting to needs work to apply the NotBlank to this as well. I'm not sure about the regex.

Actually, when thinking about this again, I don't think it makes sense for people to use a date formatter without using one of the letters that are actually transforming the date into something that has to do with the input.

So the regex, while it will be a long one, does make sense.

This will need a test to validate the regex being applied, but I would like to get a review on the approach first.

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

Well, as soon as php updates the date formatting handling. The changelog notes a change in 8.0 and 8.2, so with following minor release we'd have to recheck this?

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.

Updated the yaml file to add a cspell ignore line, to fix #14.

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

I already included this test change in the very first commit, because otherwise it was failing. But you are right, this should be expanded.

Added a kernel test that covers this, per @smustgrave.

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.

Merged 11.x back into this branch.

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.

Merged 11.x into the MR.

Removed the @needs-review-queue-bot files.

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

Pushed phpcs fix,

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.

I will change the test coverage, but I'm not sure how to change the validation error message. What would you prefer?

Yeah that's … tricky 😬 Maybe something like ? 🤔

Setting to needs review to get help with the writing of the error message.

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

Sure, adding [a-z][A-Z] is a MUCH simpler option, but that doesn't solve the question asked by @Wim Leers originally:

🤔 Shouldn't this have a minimum length of at least one? And … can't we/shouldn't we use a RegEx constraint to ensure this is using one or more of the available letters (to print year/month/day/hour/…)?

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

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:

I don't think that'd be right solution, people might want to print something like: "Happened on Fri, Feb 9 2024 around 14:00". That is currently possible, and would no longer be with that solution?

#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?

You're right - it is currently also broken, but it is what I had intended to write, to allow something like this https://3v4l.org/HIK4D. Not sure if we want to support this usecase.

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 , 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?

Right now, the constraint we have for the 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 pass this regex. It simply says you must have at least one of the characters that is supported now.

So, unless PHP 8.4 drops support for all of these characters in date formats...this pattern will continue to be valid. And rightfully so. This is already a very permissive validation. Because of that, it's also very future-proof.

Therefore, I think #11 is maybe not looking at this correctly.

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

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, it would be a perfectly legitimate reason to open an issue to add the newly-supported letter. :) That'd be a one-line fix and probably wouldn't even need test coverage.

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 😅)

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