Fixes #5788 by making sure that do_action( 'wp_insert_post' #6051
Fixes #5788 by making sure that do_action( 'wp_insert_post' #6051
Conversation
|
LGTM, @gravityrail anything that could break things here? |
| */ | ||
| function save_publicized( $post_ID, $post, $update ) { | ||
| function save_publicized( $post_ID = null, $post = null, $update = null ) { | ||
| if ( is_null( $post_ID ) || is_null( $post ) || is_null( $update ) ) { |
There was a problem hiding this comment.
Since we're not using $update in this function, and relying on the publicize meta being set, should we care if $update is null?
There was a problem hiding this comment.
I think so since the action is suppose to be called with it.
| */ | ||
|
|
||
| function expand_wp_insert_post( $args ) { | ||
| if ( ! isset( $args[0], $args[1], $args[2] ) ) { |
There was a problem hiding this comment.
Same in this file. Does it matter if the post is being updated or not?
|
This seems to be a solid change to me, with the exception that I don't think we need to worry about |
gravityrail
left a comment
There was a problem hiding this comment.
LGTM if you narrow those defaults as per comments.
| * connections. | ||
| */ | ||
| function save_publicized( $post_ID, $post, $update ) { | ||
| function save_publicized( $post_ID = null, $post = null, $update = null ) { |
There was a problem hiding this comment.
The original ticket only has warnings for arguments 2 and 3, so I suspect those are the only ones that need defaults and checks. As per Eric's message below, just having a default value for $update suppresses the warning properly, so there's no need to check its actual value.
This might be more concise:
function save_publicized( $post_ID, $post = null, $update = null ) {
if ( is_null( $post ) ) {
return;
}
}There was a problem hiding this comment.
The plugin that causes this error does the following.
do_action( 'wp_insert_post', 'wp_insert_post');
It is true that I don't need to check all the values. I could just check that $post_id is a number.
The reason why I implemented it this way was to attempt to make it more future proof.
What if a plugin comes along and does something that we don't expect. Like ignore the $update value or not set the post or just call do_action( 'wp_insert_post' );
Even though the plugin does something unexpected Jetpack should try to prevent PHP errors from showing up.
There was a problem hiding this comment.
What if a plugin comes along and does something that we don't expect.
I'd also be interested to see if we're unexpectedly breaking by checking for null in all 3 cases.
There was a problem hiding this comment.
@lezama, @ebinnion and @gravityrail Have you tried replicating the issue with the plugin. Should I write up some test to explain what is going on in order to get this merged?
| } | ||
|
|
||
| public function send_published( $post_ID, $post, $update ) { | ||
| public function send_published( $post_ID = null, $post = null, $update = null ) { |
There was a problem hiding this comment.
As per my previous comment, I don't think you need to check all these values, just:
public function send_published( $post_ID, $post = null, $update = null ) {
if ( is_null( $post ) ) {
return;
}
}…roperlyMinimal changes Fixes #5788 Added test Made the test pass with minimal assumptions.
cc229e0 to
4de4887
Compare
| */ | ||
| function save_publicized( $post_ID, $post, $update ) { | ||
| function save_publicized( $post_ID, $post = null, $update = null ) { | ||
| if ( is_null( $post ) ) { |
There was a problem hiding this comment.
Just wondering why you check for a numeric post id here https://github.com/Automattic/jetpack/pull/6051/files#diff-b42b8628b3d5d856de376e02baace310R254 but not here?
There was a problem hiding this comment.
Becasue we never use the $post_ID anywhere in that function so it doesn't matter if the value is set.
* Changelog: first pass at a changelog for 5.0 * Changelog: delete 4.9 testing list. * Changelog: update minimum WP version to match ver. in jetpack.php Fixes #7158 * Changelog: add #6051 * Changelog: add #6753 * Changelog: add #6928 * Changelog: add #6964 * Changelog: add #7014 * Changelog: add #7057 * Changelog: add #7060 * Changelog: add #7068 * Changelog: add #7070 * Changelog: add #7072 * Changelog: add #7071 * Changelog: add release date and post shortlink. * Changelog: add #7094 * Changelog: add #7100 * Changelog: add #7108 * Changelog: add #7113 * Changelog: add #7123 * Changelog: add #7135 * Changelog: add #7143 * Changelog: add #7151 * Changelog: add #6996 * Changelog: add #7105 * Changelog: add #7132 * Changelog: add #7166 * Changelog: fix typo in 4.9 changelog. * Changelog: remove older releases' changelogs. @see p1HpG7-42e-p2 * Changelog: add #7090 * Changelog: add #7095 * Changelog: add #7112 * Changelog: add #7115 * Changelog: add #7122 * Changelog: add #7137 * Changelog: add #7138 * Changelog: add #7140 * Changelog: add #7154 * Changelog: add ##7155 * Changelog: add #7163 * Changelog: add #7167 * Changelog: add #7171 * Changelog: add #7180 * Changelog: add #7181 * Changelog: add #7183 * Changelog: add #7184 * Changelog: add #7189 * Changelog: add #7191 * Changelog: add #7193 * Changelog: add #7198 * Changelog: add #7200 * Changelog: add #7209 * Changelog: add #7212 * Testing list: add instructions for #7115 * Changelog: add #7188 * Changelog: add #7205 * Changelog: add #7225 * Changelog: add #6872 * Changelog: add #7107 * Changelog: add #7118 * Changelog: add #7142 * Changelog: add #7170 * Changelog: add #7210 * Changelog: add #7218 * Changelog: add #7232 * Changelog: add #7211 * Changelog: add #7213 * Changelog: add #7229 * Changelog: add #7230 * Changelog: add #7214 * Draft changelog for 5.0 * Changelog updates: 2nd pass at a clearer changelog. - Fix typos. - Use consistent tense and tone across all changelog. - Remove unclear items. * Changelog: add #7026 * Changelog: add #7058 * Changelog: add #7125 * Changelog: add #7249 * Changelog: add #7185 * add mentions of image widget migration * Changelog: add info about new output for CLI command. * Changelog: add WP version number matching the new Image Widget.
is being called with all arguments. Otherwise bail.
Some plugins namely https://wordpress.org/plugins/pet-manager/ call
do_action in a strange way. This PR fixes any warning that would be
displayed because of the the values are not passed in as expected.
Fixes #5788
Changes proposed in this Pull Request:
Testing instructions:
and notice that the php warnings are not there any more.