Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: allow explicit 0 secfracs in datetime format #234

Merged
merged 1 commit into from
Jun 28, 2016

Conversation

vaeryn-uk
Copy link
Contributor

No description provided.

@@ -138,7 +138,7 @@ protected function validateDateTime($datetime, $format)
// which will fail the above string comparison because the passed
// $datetime may be '2000-05-01T12:12:12.123Z' but format() will return
// '2000-05-01T12:12:12.123000Z'
if ((strpos('u', $format) !== -1) && (intval($dt->format('u')) > 0)) {
if ((strpos('u', $format) !== -1) && (preg_match('/\.\d+Z/', $datetime))) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pattern will produce a match on the invalid test case yet the invalid test case passes because of the absence of the "u" check in this condition.

I have a refiddle where I added the end-of-string anchor that seems to make it a bit more clear: http://refiddle.com/3nf2

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, thanks. Also, I hadn't considered 2000-05-01T12:12:12.000000+01:00, which this will fail on.

EDIT: although that seems okay because format Y-m-d\TH:i:s.uP is never attempted.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have updated with your suggestion.

@bighappyface
Copy link
Collaborator

@mathroc what do you think?

@mathroc
Copy link
Contributor

mathroc commented Feb 22, 2016

that's working for me :)
👍

@bighappyface
Copy link
Collaborator

+1

@bighappyface bighappyface merged commit 8490e82 into jsonrainbow:master Jun 28, 2016
@erayd
Copy link
Contributor

erayd commented Mar 8, 2017

I'm trying to add unit tests to cover this (#366), as the return line is never being hit by the current testsuite. However, I have a few questions about this code:

  1. What is the purpose of the strpos conditional? As written, this will always return true. Unless this is intended to work around some kind of bug in the PHP core, strpos should always return an integer offset >= 0, or boolean false.
  2. The case cited in the comment is already in the testsuite, but never actually hits this - the relevant test case seems to be handled by add rfc3339 helper class #238, which was merged several months prior to this PR. I assume that means there's a reason for this that isn't covered by the rfc3339 helper thing, but I can't figure out what that might be.
  3. This code appears to be intended for checking date-time constraints, but that constraint is entirely handled by the rfc3339 helper. Does this code perhaps need to be moved and integrated with that?

I would appreciate it if someone could clarify what this code is actually for, and whether it still serves a purpose, needs moving, has been superseded, or was simply merged in error after the problem was already solved.

        // handles the case where a non-6 digit microsecond datetime is passed
        // which will fail the above string comparison because the passed
        // $datetime may be '2000-05-01T12:12:12.123Z' but format() will return
        // '2000-05-01T12:12:12.123000Z'
        if ((strpos('u', $format) !== -1) && (preg_match('/\.\d+Z$/', $datetime))) {
            return true;
        }

@erayd erayd mentioned this pull request Mar 8, 2017
@guilliamxavier guilliamxavier mentioned this pull request Feb 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants