Properly return FALSE for invalid strings in DateTimePlus::checkArray() so that the correct exception is thrown

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

This will need a test case to show the issue as a next step.

Thanks

added patch against 10.1.x and fixed some coding standard issues

Fix command failures #7.

Adding Reroll for 10.1.x

Please see the tags before moving status this needs tests

CCF fixes found in #9

The patch from #2 was converted to an MR. Later patches included unrelated code style changes. I also added new cases to the existing test data provider.

Think if someone incorrectly passes a string we should maybe log or throw an error so people know they need to fix their code.

Think if someone incorrectly passes a string we should maybe log or throw an error so people know they need to fix their code.

I disagree. This is a validation function. It seems inappropriate for validation to throw an error if something doesn't validate. Logging, maybe, but I think even that's a stretch. It should be up to calling code to decide what to do if validation doesn't pass.

The only place in Core where this is called is in DateTimePlus::createFromArray() in the same class. ::createFromArray() throws an exception if validation failed. So it's doing the job.

I'll leave this one for others to review then.

Since no one else has reviewed in 2 months and my hesitation isn't something I feel so strong about to block I'm going to mark as the fix is straight forward.

At first I was confused by this, because I was expecting the date parts to allow valid month formats like "January", but it does indeed expect it to be integers.

There's an argument to be made that checkArray() should be a protected method, for the reason in #17.

Since int is expected, I am not sure is_numeric() goes far enough, since that could be floats and whatnot. It should be an int, or a string whose numeric value is an int.

I agree letting the caller handle throwing the exception is fine, and let's open a followup to discuss whether we should deprecate this being public and make it protected next major.

Thanks!

Retitling to make it clear we're not "handling" them (which would be brittle and hide bugs). We're returning FALSE to trigger an exception in the caller. The original title had me all set to NW it but the approach is actually sound.

Feedback has been addressed.

Mind opening the follow up ticket mentioned in #20

The follow-up was created at #3566186: Make DateTimePlus::checkArray() protected.

Thanks rest LGTM!

So this change results in a slightly different behaviour. It's a bit of an edge case but if you call the function with a float
On HEAD

> (string) \Drupal\Component\Datetime\DateTimePlus::createFromArray(['year' => 1975.5]);

   DEPRECATED  Implicit conversion from float 1975.5 to int loses precision in core/lib/Drupal/Component/Datetime/DateTimePlus.php on line 577.

= "1975-01-01 00:00:00 Europe/London"

With MR

> (string) \Drupal\Component\Datetime\DateTimePlus::createFromArray(['year' => 1975.5]);

   InvalidArgumentException  The array contains invalid values.

However I think is is okay because with the MR if you do

> (string) \Drupal\Component\Datetime\DateTimePlus::createFromArray(['year' => 1975.0]);
= "1975-01-01 00:00:00 Europe/London"

It's fine so it'll still accept floats but not ones that don;t represent an integer and passing a value that is not an integer to day, month or year makes no sense. However given this very slight behaviour change I'm going to only merge to main and 11.x

Committed and pushed 22f99645e25 to main and 108cf90c63c to 11.x. Thanks!