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: DateTimePicker passes current seconds as the selected seconds value #15495

Conversation

@jorgefilipecosta
Copy link
Member

commented May 8, 2019

Closes: #13898

DateTimePicker does not allow users to pick a value for seconds, so when the user changes a time/date, it should look like a value of 0 was selected as the seconds value. Currently, the seconds of the current date are passed as the selected seconds, which may make it look like a "random" value was selected as the seconds value.

This causes a regression where WP_CRON missed some scheduled posts.

How has this been tested?

I created a new post.
I changed the date/time fields in the post scheduling dialog and I verified with redux dev tools that the editPost action contained a date whose seconds were equal to 0.

@gziolo

This comment has been minimized.

Copy link
Member

commented May 10, 2019

This PR needs to be rebased with master to make Travis green again :)

@jorgefilipecosta jorgefilipecosta force-pushed the fix/dateTimePicker-passes-current-seconds-as-the-selected-value-for-seconds branch 2 times, most recently from e2a710f to d1b1f66 May 10, 2019

@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

commented May 13, 2019

Hi @gziolo this PR was rebased.

@mapk

This comment has been minimized.

Copy link
Contributor

commented May 30, 2019

This is great! Thanks for fixing @jorgefilipecosta!

@katsar0v

This comment has been minimized.

Copy link

commented Jun 4, 2019

Any update on this?

@noisysocks

This comment has been minimized.

Copy link
Member

commented Jun 6, 2019

This causes a regression where WP_CRON missed some scheduled posts.

Could you elaborate on this? What causes cron to miss the posts?

@katsar0v

This comment has been minimized.

Copy link

commented Jun 6, 2019

Could you elaborate on this? What causes cron to miss the posts?

I explained it here - #13898 (comment)
But will try again. There are missed schedules (e.g.: https://www.hostinger.com/tutorials/how-to-fix-wordpress-missed-schedule) and there is this famous plugin which fixes it all: https://github.com/sLaNGjI/wp-missed-schedule

Use case: I setup wordpress to use system cron (setup a cronjob calling the wp-cron every minute for example). After setting up the cronjob I plan a post for the next week on 20:00. The time in database stored is 20:00:43 (43 in this case is seconds, fully random, but in fact this are my browser seconds).
Now the cronjob runs every minute and starts on the 0th second. If the cronjob is calling the url via http it might take longer than calling the wp-cron.php via php process (but that's an edge case). When the cron job starts next week on 20:00 the post is not being published, because time is 20:00:01 (time of cron job execution) but the post should be published on 20:00:43 (remember the 43 seconds?).
Now the next time the cron job runs (on 20:01) the post does not get published as well and wordpress says "missed schedule" which really annoys me and probably thousands of other WP users...

@aduth

aduth approved these changes Jun 6, 2019

Copy link
Member

left a comment

Looks good. A nice refactor of some duplicate logic too 👍

@@ -62,6 +63,12 @@ class TimePicker extends Component {
}
}

changeDate( newDate ) {

This comment has been minimized.

Copy link
@aduth

aduth Jun 6, 2019

Member

Might be nice for some docs, even if there's plenty of prior art of no documentation in this component.

@jorgefilipecosta jorgefilipecosta force-pushed the fix/dateTimePicker-passes-current-seconds-as-the-selected-value-for-seconds branch from d1b1f66 to c270593 Jun 7, 2019

@jorgefilipecosta jorgefilipecosta force-pushed the fix/dateTimePicker-passes-current-seconds-as-the-selected-value-for-seconds branch from c270593 to 62c0af3 Jun 7, 2019

@jorgefilipecosta jorgefilipecosta merged commit 0fff1c5 into master Jun 7, 2019

1 of 2 checks passed

Filter merged Filter merged
Details
Travis CI - Pull Request Build Passed
Details

@youknowriad youknowriad added this to the Gutenberg 5.9 milestone Jun 7, 2019

nicolad added a commit to nicolad/gutenberg that referenced this pull request Jun 15, 2019

Fix: DateTimePicker passes current seconds as the selected seconds va…
…lue (WordPress#15495)

DateTimePicker does not allow users to pick a value for seconds, so when the user changes a time/date, it should look like a value of 0 was selected as the seconds value. Currently, the seconds of the current date are passed as the selected seconds, which may make it look like a "random" value was selected as the seconds value.

This causes a regression where WP_CRON missed some scheduled posts.

jg314 added a commit to jg314/gutenberg that referenced this pull request Jul 19, 2019

Fix: DateTimePicker passes current seconds as the selected seconds va…
…lue (WordPress#15495)

DateTimePicker does not allow users to pick a value for seconds, so when the user changes a time/date, it should look like a value of 0 was selected as the seconds value. Currently, the seconds of the current date are passed as the selected seconds, which may make it look like a "random" value was selected as the seconds value.

This causes a regression where WP_CRON missed some scheduled posts.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.