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 datepickers to show correct minDate #18524

Merged
merged 1 commit into from
Sep 20, 2020

Conversation

MegaphoneJon
Copy link
Contributor

@MegaphoneJon MegaphoneJon commented Sep 18, 2020

https://lab.civicrm.org/dev/core/-/issues/2052

(This is against the RC, otherwise identical to #18522)

Overview

#16610 enforces a requirement that datepickers pass dates in Y-m-d format. Some datepickers currently pass a Unix timestamp.

Before

At certain times of day, these datepickers show a minimum year thousands of years in the future.

After

Minimum year is 2020 (and counting).

Comments

Depending on the time of day, this bug won't manifest in the UI. If the code in #16610 results in a date like "0255-09-18" then it's ignored. To test if the bug is present, check the value of $extra['minDate'] after it passes through this line from #16610. If the date isn't today's date, the bug is present.

@civibot civibot bot added the master label Sep 18, 2020
@civibot
Copy link

civibot bot commented Sep 18, 2020

(Standard links)

@colemanw
Copy link
Member

I thought that #16610 was passing the min and max dates through strtotime(). Does that function not work with timestamps?

@MegaphoneJon
Copy link
Contributor Author

Evidently not, hence the bug. From the docs: "The function expects to be given a string containing an English date format and will try to parse that format into a Unix timestamp"

@colemanw colemanw changed the base branch from master to 5.30 September 19, 2020 23:54
@civibot civibot bot added 5.30 and removed master labels Sep 19, 2020
@colemanw
Copy link
Member

Ok. This works. Theoretically the string "now" would also work. But this is fine. Thank you for the fix.
I've changed the base to 5.30 because I agree this is a regression we should fix in the RC.

@eileenmcnaughton
Copy link
Contributor

Unrelated fail I believe

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