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 'No date part in '' found. Upgrade from 1.9.0.x to 1.9.2.2 #923

Closed
wants to merge 2 commits into from

Conversation

@sreichel
Copy link
Contributor

I'd not merge this.

The error No date part in is correct, because 0000-00-00 00:00:00 is not a valid date.

Even ...

sql> UPDATE `prefix_customer_entity` SET created_at =  "0000-00-00 00:00:00"

Results in ....

Incorrect datetime value: '0000-00-00 00:00:00' for column 'created_at' at row 1

@sreichel sreichel dismissed colinmollenhour’s stale review May 11, 2020 21:25

This should be fixed outside core?

@colinmollenhour
Copy link
Member

That may be an invalid date, but this patch will avoid there being a PHP warning. That is, the execution result for the case where the value being parsed is an empty string is the same, just without the PHP warning.

@sreichel
Copy link
Contributor

sreichel commented May 11, 2020

If created_at is empty or invalid something went wrong before. (imho)

@colinmollenhour
Copy link
Member

If I recall correctly I've seen this error triggered many times from things like automated scanners. It is annoying that it throws PHP warnings because those get logged in a more urgent way for me so it is like a false alarm whereas fixing the PHP warning lets the error just bubble up to the user and I can happily ignore it. :)

@sreichel
Copy link
Contributor

I'll not approve this. Sorry.

@colinmollenhour
Copy link
Member

@sreichel Is this even related to 0000-00-00 00:00:00? If it was, wouldn't the error be "No date part found in '0000-00-00 00:00:00'"?

@sreichel
Copy link
Contributor

You're right, but even an empty value is not intended .... the field isn't nullable.

Instead of "fixing" the output, i'd prefer input validation. (dont know where these wrong values come from)

@seansan
Copy link
Contributor Author

seansan commented May 14, 2020 via email

@sreichel
Copy link
Contributor

So maybe both views are true?

Of course! From merchants view i'm with you, but imho it is still wrong. Howerever, i'll neither approve nor block this.

@colinmollenhour
Copy link
Member

Ahh, I just pushed a commit and then realized that it actually doesn't fix anything because the $splitted[0] will still be an array even if there were no matches.. I was thinking that $splitted[0] would be an undefined index. Sorry for the confusion... I think maybe a better solution is just to track down individual issues where an empty string is parsed and fix them there. I'll push PR for the one in the stackexchange thread shortly.

@colinmollenhour
Copy link
Member

@seansan please see #980, does that address the issue in your view?

@seansan
Copy link
Contributor Author

seansan commented May 15, 2020 via email

@colinmollenhour
Copy link
Member

@seansan I think any code accessing created_at where it is not guaranteed to have a valid value should check first if the value is not null before trying to parse it. So I think there is a strong case that it is a bug in the extension code. I don't think we should change the behavior of a core library that attempting to parse an empty string throws an exception.

edannenberg pushed a commit to edannenberg/magento-lts that referenced this pull request Aug 20, 2020
edannenberg pushed a commit to edannenberg/magento-lts that referenced this pull request Aug 24, 2020
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.

3 participants