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

[5.8] Use date_create to prevent unsuppressible date validator warnings #29342

Merged
merged 1 commit into from Jul 30, 2019

Conversation

@LarsGrevelink
Copy link
Contributor

commented Jul 30, 2019

Due to an unresolved PHP bug warnings are being triggered which cannot be caught or suppressed. The warning occurs when using a different property is used which is not parseable by the new DateTime instance. Applications like NewRelic pick up on these warnings and are logging them while there is no direct inconvenience for the users.

Replaced the new DateTime by date_create which is an alias for the DateTime constructor but silences these kinds of warnings.

Fixes issue #27784

@taylorotwell taylorotwell merged commit ed3edd0 into laravel:5.8 Jul 30, 2019

2 checks passed

continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@GrahamCampbell

This comment has been minimized.

Copy link
Member

commented Jul 30, 2019

Are there any other places where this is relevant?

@LarsGrevelink

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2019

I couldn't find another place where this specific case would occur in the repo. I haven't checked the other Laravel products though. We've been keeping a close eye on our tooling to check for other occurrences and none seemed to happen after patching this bad boy.

@mfn

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2019

@LarsGrevelink that's quite interesting, how did you find out about this? 😄

@LarsGrevelink

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2019

@mfn We're having quite some date-related rules on our platform. NewRelic started reporting these warnings. That triggered further research which eventually led to the fix we applied through this PR.

@mfn

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2019

@LarsGrevelink I took a second look and found two small issues/concerns:

  • date_create returns false for invalid dates but the method signature does not tell so. Personally I think it would be preferred to coerce that to null anyway
  • Is catching the exception now still relevant? DateTime was throwing one, date_create isn't. The code before that seems to have checks in place, unclear if the exception would apply to it
$ php -r '$a = new DateTime("Invalid"); var_dump($a);'
PHP Fatal error:  Uncaught Exception: DateTime::__construct(): Failed to parse time string (Invalid) at position 0 (I): The timezone could not be found in the database in Command line code:1
Stack trace:
#0 Command line code(1): DateTime->__construct('Invalid')
#1 {main}
  thrown in Command line code on line 1

vs

$ php -r '$a = date_create("Invalid"); var_dump($a);'
bool(false)
@driesvints

This comment has been minimized.

Copy link
Member

commented Aug 2, 2019

@mfn Date::parse still throws an exception I think? That's still in the try/catch block. We should indeed convert the false to null before returning.

@LarsGrevelink

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2019

Will create an additional PR converting any falsy values to null on returning the value. Personal preference is to leave the structure as is, but if you want to isolate the Date::parse let me know and I will take that along. Have a great weekend!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.