Fix Issue 1441 (isDateTime and Formulas) #1480
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Resync with base project
Sync with Base
When you have a date-field which is a formula, isDateTime returns false. #1441 Report makes sense; fixed as suggested. Also fixed a few minor related issues, and added tests so that Shared/Date and Shared/TimeZone are now completely covered. Date/setDefaultTimeZone and TimeZone/setTimeZone were not consistent about what to do in event of failure - return false or throw. They will now both return false, which is what Date's function said it would do in its doc block anyhow. Date/validateTimeZone will continue to throw; it was protected, but was never called outside Date, so I changed it to private. TimeZone/getTimeZoneAdjustment checked for 'UST' when it probably meant 'UTC', and, as it turns out, the check is not even needed. The most serious problem was that TimeZone/validateTimeZone does not check the backwards-compatible time zones. The timezone project aggressively, and very controversially, "demotes" timezones; such timezones eventually wind up in the PHP backwards-compatible list. We want to make sure to check that list so that our applications do not break when this happens. I bundled into this change two unrelated testing changes. I think one of my Html tests could be more bulletproof in the event that a specific assertion failed. One of the sample CSV's used backslash before quote for escaping, but, in fact, Excel doubles the quote. This did not invalidate the test, but it makes the file look weird. An additional assertion was added to ensure that the cell's contents are correct.
Test members only.
Minor issue in test case. It said $cell could be null. So I put in assertNotNull($cell). Didn't help. I will try assertTrue(!is_null($cell) && ...) If this doesn't fix it, I give up. Hopefully won't affect decision whether or not to merge.
It wants null !== $cell rather than !is_null($cell). In my last commit, I said it was my last attempt to satisfy Scrutinizer. Travis hasn't been a problem, so I will do as it says.
Solution is silly, but seemed effective. We'll see.
|
Thank you for the submission Is it possible to unbundled the two unrelated testing changes, and create separate PRs for each of them? |
Per comment from Mark Baker.
|
Done. The testing changes have been removed from this PR. I will submit them, plus some other testing changes, through other PRs over the next couple of days. |
|
Thank you for this, and for the unbundling as requested |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is:
Checklist:
Why this change is needed?
When you have a date-field which is a formula, isDateTime returns false.
#1441
Report makes sense; fixed as suggested. Also fixed a few minor
related issues, and added tests so that Shared/Date and Shared/TimeZone
are now completely covered.
Date/setDefaultTimeZone and TimeZone/setTimeZone were not consistent
about what to do in event of failure - return false or throw.
They will now both return false, which is what Date's function
said it would do in its doc block anyhow. Date/validateTimeZone will
continue to throw; it was protected, but was never called outside
Date, so I changed it to private.
TimeZone/getTimeZoneAdjustment checked for 'UST' when it probably
meant 'UTC', and, as it turns out, the check is not even needed.
The most serious problem was that TimeZone/validateTimeZone does not
check the backwards-compatible time zones. The timezone project
aggressively, and very controversially, "demotes" timezones;
such timezones eventually wind up in the PHP backwards-compatible list.
We want to make sure to check that list so that our applications do not
break when this happens.
I bundled into this change two unrelated testing changes.
I think one of my Html tests could be more bulletproof in the event
that a specific assertion failed.
One of the sample CSV's used backslash before quote for escaping,
but, in fact, Excel doubles the quote. This did not invalidate the
test, but it makes the file look weird. An additional assertion
was added to ensure that the cell's contents are correct.