Improve interception functions.#4703
Conversation
|
|
||
| if ( empty( $post_id ) ) { | ||
| $post_ID = (int) $_GET['post']; | ||
| $post_id = $post_ID; |
There was a problem hiding this comment.
Ha, I also thought about what the situation might be. Couldn't think of anything but a hand-crafted invalid edit post URL. 🤔 🤷♂️
There was a problem hiding this comment.
Just a bit silly to not assign this variable directly. But well...
There was a problem hiding this comment.
Oh, you mean why there are two variables...? Well, these are actually two different globals. See the import at the top of the function.
gutenberg.php
Outdated
| $post_ID = (int) $_GET['post']; | ||
| $post_id = $post_ID; | ||
|
|
||
| if ( ! $post_id ) { |
There was a problem hiding this comment.
When would this condition be triggered? Looks like dead code to me.
There was a problem hiding this comment.
Yeah, this is what @swissspidy and I were talking about. I only can think of incorrect usage of the functionality, for example, hand-crafting an invalid URL without a (valid) post ID.
There was a problem hiding this comment.
Empty value, negative number, zero, some string, ... All that would be (int) 0, and thus falsy.
There was a problem hiding this comment.
Ok, I understand, but when would $post_id be assigned zero, since it's indirectly assigned from $_GET['post'] and we have the empty( $_GET['post'] ) condition above to shortcut?
There was a problem hiding this comment.
When I put ?post=foo in the URL, its not empty in terms of the first check empty(), but after casting it to an integer, it is 0, and thus empty.
There was a problem hiding this comment.
But yes, personally, I would have adapted the first check to include this already. This would have been a backward incompatible change, though, as the current code sets the $post_id and $post_ID variables to that invalid value.
Should I adapt this?
There was a problem hiding this comment.
if ( empty( $_GET['post'] ) || ! is_numeric( $_GET['post'] ) ) {
return;
}
$post_ID = (int) $_GET['post'];
$post_id = $post_ID;
There was a problem hiding this comment.
Yes, that seems improved to me, since if this conversation is any evidence, what was being accounted for was not entirely clear (though in retrospect makes sense, thanks for clarifying).
Re: Backwards incompatibility, I'd assume post=foo would break miserably in some other manner anyways, or at least not something we'd intend to support.
| } | ||
|
|
||
| if ( empty( $post_id ) ) { | ||
| if ( empty( $_GET['post'] ) || ! is_numeric( $_GET['post'] ) ) { |
There was a problem hiding this comment.
In retrospect, ?post=0.1 would pass this check and be coerced into a 0 int value, which is what the previous condition was guarding against. However, I'd guess this would make it as far as the $post assignment and fail anyways on the subsequent falsey-ness check. Does that sound right to you, and then a non-concern?
There was a problem hiding this comment.
Yes, but there is no way that 0.1 could come from the database (ID is a BIGINT), so this is invalid data anyway. And also, yes, this would still be some (invalid) edge case. -1 would also pass, but in any case, the check for the post object further down will fail, and all is good, I'd say.
|
You may need to rebase your branch against master and force push your branch to resolve the build errors. |
|
No, there are no conflicts. You just need to restart the build (the single one that failed). I can't do that as I am not a member/owner of this repository. |
|
Restarted it now :-) |
|
All green. Go, go, gooo! 😅 |
Hmm, I had restarted the build prior to leaving my comment and it was still failing. Not sure why. |
Description
Overall improved version of the
gutenberg_intercept_*functions.Types of changes
in_array).$post_type.Checklist: