-
Notifications
You must be signed in to change notification settings - Fork 90
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
Prevent unexpected missed schedules when generating new posts #418
Prevent unexpected missed schedules when generating new posts #418
Conversation
* maybe_convert_hyphenated_date_format( "2018-07-05-17:17:17" ); | ||
* Returns: "2018-07-05 17:17:17" | ||
*/ | ||
private function maybe_convert_hyphenated_date_format( $date_string ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a new behavior? Can you share more detail about it? It seems like a new behavior to me, and a different change than the original issue describes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it's actually done to ensure compatibility with previous versions. The docs advises using a datetime string with a hyphen between the date and time. If we don't remove this hyphen before calling wp_insert_post
, the timestamp will be saved as 0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rafaelzaleski Ok, great. Can you include some additional tests to capture this behavior then?
Also, it looks like your test introduces one break in a different test...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rafaelzaleski Ok, great. Can you include some additional tests to capture this behavior then?
Also, it looks like your test introduces one break in a different test...
I'm not sure how to reproduce it locally. Do you have any ideas? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how to reproduce it locally. Do you have any ideas?
I tried to get tests running against PHP 5.6 locally and failed 😕
First, I tried this act tool. It was able to run the unit tests but not the functional tests:
I think this is the specific failure:
Next, I tried to install PHP 5.6 directly on my machine:
brew install shivammathur/php/php@5.6
brew link --overwrite --force shivammathur/php/php@5.6
Something is funky with the tests, though:
I then manually tried to install WordPress 3.7, but received this MySQL error:
My guess is that something changed in the behavior of wp_insert_post()
between WP 3.7 and today.
Based on the failed test, it looks like your code changes the result of --post_date="2018-07-01"`` from
2018-07-01 00:00:00to
1970-01-01 00:00:00`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @danielbachhuber. I think I was able to fix the edge cases on WP 3.7 and I have requested a new review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I was able to fix the edge cases on WP 3.7
@rafaelzaleski Out of curiosity, how did you end up debugging/fixing this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rafaelzaleski Out of curiosity, how did you end up debugging/fixing this?
I encountered some challenges installing 3.7
locally too, mainly because of database issues.
So I went through the failed test data and reviewed the wp_insert_post
source code from tag 3.7
. This helped me understand the nuances of the test data processing and the root cause of the failures.
To ensure the solutions were working, I validated them using CI checks on my cloned repository.
wp_insert_post
behavior for post_date
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work on this, @rafaelzaleski 👏
This PR uses an empty string as the default value for the
--post_date
and--post_date_gmt
parameters ofwp generate posts
.This will align with the behavior of
wp_insert_post
and use the current time for the generated posts when a date is not provided. If a date is provided, it's passed down towp_insert_post
.Closes #268
Related wp-cli/wp-cli#5832