Open oschwald opened 8 months ago
Are you sure that it's working on version 2.2? I'm getting an exception when I runt the code below:
v::dateTime(\DateTime::RFC3339)->assert('2014-04-12T23:20:50.052Z')
v::dateTime(\DateTime::RFC3339)->assert('2005-12-30T01:02:03+00:00')
This works fine on 2.3
If you use PHP's DateTime constant as a format, you must check its value. See DateTimeInterface
RFC3339 = "Y-m-d\\TH:i:sP"
RFC3339_EXTENDED = "Y-m-d\\TH:i:s.vP"
It is possible to validate that the way you want, but you must use a different format: Y-m-d\TH:i:s.vp
. It's very similar to RFC3339_EXTENDED
, but it used p
instead of P
. You can check DateTime.format for more details.
I wouldn't like to overwrite PHP's default behaviour because that could cause more confusion. If I misunderstood your request, feel free to reopen the issue. I'm here to help!
Cheers 🐼
Yes, this was working on 2.2. The tests for the above repo pass with 2.2 and fail with 2.3. We have weekly automated builds that were passing until last week. I also just tested this downgrading to 2.2.
DateTime
can parse this fine:
echo DateTime::createFromFormat(DateTime::RFC3339_EXTENDED, '2014-04-12T23:20:50.052Z')->format("H:i:s.v");
outputs 23:20:50.052
.
I bisected the issue to 5fe4b96eb.
The issue is $value !== $formattedDate->format($format)
as 2014-04-12T23:20:50.052Z
will be turned into 2014-04-12T23:20:50.052+00:00
.
I wouldn't call this an "issue", although I understand that that can be problematic for your use case.
The commit you mentioned aimed to fix an issue with the date-related rules. See #1274.
Our date-related rules aim to respect the format that we pass to them. The value of DateTimeInterface::RFC3339
is Y-m-d\\TH:i:sP
, while the input in your example matches the Y-m-d\TH:i:s.vp
format.
This is a breaking change. It accepted valid RFC 3339 timestamps before the change with the PHP RFC3339 format and now it rejects them.
On Fri, Feb 2, 2024, 3:27 AM Henrique Moody @.***> wrote:
I wouldn't call this an "issue", although I understand that that can be problematic for your use case.
The commit you mentioned aimed to fix an issue with the date-related rules. See #1274 https://github.com/Respect/Validation/issues/1274.
The date-related aim is to respect the format that we pass to them. The value of DateTimeInterface::RFC3339 is Y-m-d\TH:i:sP, while the input in your example matches the Y-m-d\TH:i:s.vp format.
— Reply to this email directly, view it on GitHub https://github.com/Respect/Validation/issues/1442#issuecomment-1923614701, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACECMYKAIIEPK3NBV7RTB3YRTESTAVCNFSM6AAAAABCPVUGWOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMRTGYYTINZQGE . You are receiving this because you authored the thread.Message ID: @.***>
I understand, and I'm sorry you are frustrated because your code doesn't work as expected. After all, that happened because we changed things, not you.
I insist that when you pass a format to the DateTime rule, it expects the input to match the format you pass to it. From the perspective of the rule, the value you're passing is Y-m-d\\TH:i:sP
and not RFC3339.
In fact, PHP itself cannot parse all dates using that constant, even the ones mentioned in the RFC itself. See https://onlinephp.io/c/4609f
Reverting that commit will defeat the purpose of our date-related rules, and I will not do that. That change makes the rules more predictable and accurate.
I suggest you add another rule to your chain, so it will accept yet another format v::dateTime('Y-m-d\TH:i:s.vp')
. That will fix the issue you're having.
If you look at the commit that mentions this issue, I did that. However, users of my library that happen to have a newer version of Respect/Validation
with an older version of the library are still experience the breakage. The RFC3339 format intentionally allows Z
while parsing as it is a valid representation
How do you suggest I deal with the other cases where this breaking change caused valid timestamps to fail, e.g., fractional seconds? For instance, 2014-04-12T23:20:50.05+00:00
. It is a mistake to assume that a valid timestamp will round-trip to exactly the same string.
You need to include formats you intend your rule to pass. If Y-m-d\\TH:i:sP
(DateTime::RFC3339
) and Y-m-d\\TH:i:s.vP
(DateTime::RFC3339_EXTENDED
) are not sufficient, you might want to add the formats you want to allow.
diff --git a/src/MinFraud/Validation/Rules/Event.php b/src/MinFraud/Validation/Rules/Event.php
index ea2a33c..3639b50 100644
--- a/src/MinFraud/Validation/Rules/Event.php
+++ b/src/MinFraud/Validation/Rules/Event.php
@@ -20,7 +20,9 @@ class Event extends AbstractWrapper
'time',
v::anyOf(
v::dateTime(\DateTime::RFC3339),
- v::dateTime(\DateTime::RFC3339_EXTENDED)
+ v::dateTime(\DateTime::RFC3339_EXTENDED),
+ v::dateTime('Y-m-d\TH:i:s.vp'),
+ v::dateTime('Y-m-d\TH:i:sp'),
),
false
),
Perhaps I didn't understand you. I saw you commit https://github.com/maxmind/minfraud-api-php/commit/a23246ff7cd865d40e7e5d9b81b4e294f4d54a1f, and you did exactly what I did.
However, users of my library that happen to have a newer version of Respect/Validation with an older version of the library are still experience the breakage.
I understand that this may be upsetting, but the change introduced was, in fact, a bug fix. The problem was that your code used the bug in its favour, as it was more flexible. If I revert the change, there could be other people experiencing a "real" bug.
How do you suggest I deal with the other cases where this breaking change caused valid timestamps to fail, e.g., fractional seconds? For instance, 2014-04-12T23:20:50.05+00:00. It is a mistake to assume that a valid timestamp will round-trip to exactly the same string.
That's what I don't think I understood. 2014-04-12T23:20:50.05+00:00
matches Y-m-d\TH:i:s.vp
perfectly.
The RFC3339 format intentionally allows Z while parsing as it is a valid representation
It does, and I understand that. However, neither DateTime::RFC3339
nor DateTime::RFC3339_EXTENDED
match that format.
I understand that this may be upsetting, but the change introduced was, in fact, a bug fix. The problem was that your code used the bug in its favour, as it was more flexible. If I revert the change, there could be other people experiencing a "real" bug.
I think this is confusing parsing with formatting.
That's what I don't think I understood. 2014-04-12T23:20:50.05+00:00 matches Y-m-d\TH:i:s.vp perfectly.
Yes, like Z
with P
, it matches the format for parsing and it does not match the string produced with that format. Given:
<?php
require 'vendor/autoload.php';
use Respect\Validation\Validator as v;
v::dateTime(\DateTime::RFC3339_EXTENDED)->assert('2014-04-12T23:20:50.05+00:00');
I receive the following error on 2.3:
PHP Fatal error: Uncaught All of the required rules must pass for "2014-04-12T23:20:50.05+00:00"
thrown in /home/greg/MaxMind/minfraud-api-php/vendor/respect/validation/library/Factory.php on line 233
As mentioned above, this is due to the use of the fractional second. PHP formats it as three digits, i.e., 2014-04-12T23:20:50.050+00:00
, which does not match the original string.
I see a distinction between parsing and formatting, but remember that the date-related rules validate whether the input matches a given format and NOT if the input can be parsed with DateTime::createFromFormat()
with a given format.
The DateTime::format()'s documentation states that
P
=> Difference to Greenwich time (GMT) with colon between hours and minutes
p
=> The same as P
, but returns Z
instead of +00:00
(available as of PHP 8.0.0)
If we break down the last date/time you provided, 2014-04-12T23:20:50.05+00:00
, we have the following:
Value | Format | Description |
---|---|---|
2014 |
Y |
A full numeric representation of a year |
04 |
m |
Numeric representation of a month, with leading zeros |
12 |
d |
Day of the month, two digits with leading zeros |
23 |
H |
24-hour format of an hour with leading zeros |
20 |
i |
Minutes with leading zeros |
50 |
s |
Seconds with leading zeros |
05 |
v |
Milliseconds |
+00:00 |
P |
Difference to Greenwich time (GMT) with a colon between hours and minutes NOT using Z instead of +00:00 |
The DateTimeImmutable::createFromFormat()'s documentation states:
The formatting constants as used in this example consist of a string of characters for formatting a DateTimeImmutable object. In most cases, these letters match with the same elements of date/time information as the ones defined in the parameters section above, but they tend to be more lenient.
I think the value of DateTimeInterface::RFC3339_EXTENDED
should have been Y-m-d\TH:i:s.vp
to account for Zulu (Z) suffix.
It is good that DateTimeImmutable::createFromFormat()
tends to be more lenient, but if someone uses v::dateTime()
, they expect an input to match a specific format. They might not use DateTime::createFromFormat()
to parse that input but expect that Validation has validated it accordingly. That would mean they'll have a false positive.
That said, I insist that this is not a bug but the expected behaviour of Validation. I will not revert the changes we made on version 2.3
Alternatively, you might use v::callback(fn($input) => is_string($input) && DateTime::createFromFormat(DateTime::RFC3339_EXTENDED, $input) !== false)
Hi @oschwald!
It took me a while to come around, but I can see now how this is a breaking change. I might need your opinion about this, because it's been a month since I released version 2.3. I could revert this change, but that means it will also be a breaking change for people using version 2.3. Do you have any suggestions?
Before 2.3, this code using
\DateTime::RFC3339
accepted RFC 3339 timestamps with the Zulu (Z
) suffix, e.g.,2014-04-12T23:20:50.052Z
. As of 2.3, an exception with the messagetime must be a valid date/time in the format "2005-12-30T01:02:03+00:00"
is thrown.I previously mentioned this in a comment. I probably should have created a separate issue, but I didn't think much of it after initially testing out the pre-release version.