Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#52995 closed defect (bug) (fixed)

Undefined `$post_ID` variable in `edit-form-blocks.php`

Reported by: ravipatel's profile ravipatel Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 5.8 Priority: normal
Severity: normal Version: 5.0
Component: Editor Keywords: has-patch
Focuses: Cc:

Description (last modified by davidbaumwald)

wp_get_post_autosave is called with the $post_ID variabl, but it is not defined. Should use $post->ID instead.

Attachments (2)

52995-edit-form-blocks.php.patch (584 bytes) - added by ravipatel 3 years ago.
undefine variable
52995.diff (1.8 KB) - added by SergeyBiryukov 3 years ago.

Download all attachments as: .zip

Change History (11)

@ravipatel
3 years ago

undefine variable

#1 @davidbaumwald
3 years ago

  • Component changed from General to Editor
  • Description modified (diff)
  • Focuses coding-standards removed
  • Summary changed from Coding standard : Undefined varible to Undefined `$post_ID` variable in `edit-form-blocks.php`
  • Version set to 5.0

Looks like this traces back to [43815].

#2 @mukesh27
3 years ago

Hi there!

I investigate more on this issue and found that $post_ID is not defined on the current file but it defines in their parent files.

edit-form-blocks.php has two parent files.

File post-new.php define $post_ID like below.

$post    = get_default_post_to_edit( $post_type, true );
$post_ID = $post->ID;

File post.php define $post_ID like below.

if ( isset( $_GET['post'] ) && isset( $_POST['post_ID'] ) && (int) $_GET['post'] !== (int) $_POST['post_ID'] ) {
	wp_die( __( 'A post ID mismatch has been detected.' ), __( 'Sorry, you are not allowed to edit this item.' ), 400 );
} elseif ( isset( $_GET['post'] ) ) {
	$post_id = (int) $_GET['post'];
} elseif ( isset( $_POST['post_ID'] ) ) {
	$post_id = (int) $_POST['post_ID'];
} else {
	$post_id = 0;
}
$post_ID = $post_id;

FYI both the variables give the same post ID value in my test.

echo $post_ID;
echo $post->ID;

#3 @SergeyBiryukov
3 years ago

  • Milestone changed from Awaiting Review to 5.8

Hi there, thanks for the ticket!

As noted above, this is not an issue, as the variable is defined in the parent file before edit-form-blocks.php is included. That said, I think it makes sense to switch to $post->ID here avoid any future confusion.

Note: This also applies to a similar fragment and a few other instances in edit-form-advanced.php.

#4 @SergeyBiryukov
3 years ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from new to closed

In 50693:

Editor: Use a consistent way to retrieve post ID on Edit Post screens.

Props mukesh27, ravipatel, davidbaumwald.
Fixes #52995.

#5 follow-up: @grantmkin
3 years ago

I believe this caused an unintended change, $post->ID can be a string in some cases, and it's used in strict comparison with an int here: https://github.com/WordPress/wordpress-develop/blob/c00ba50d598abb1cd8aaba057da9555a452ffcee/src/wp-admin/edit-form-advanced.php#L81.

This prevents the notice from showing when editing the posts page (https://github.com/WordPress/wordpress-develop/blob/c00ba50d598abb1cd8aaba057da9555a452ffcee/src/wp-admin/includes/template.php#L2668).

#6 in reply to: ↑ 5 @SergeyBiryukov
3 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Replying to grantmkin:

I believe this caused an unintended change, $post->ID can be a string in some cases

Thanks for flagging that! I was able to reproduce the issue and am looking into it.

@SergeyBiryukov
3 years ago

#7 @SergeyBiryukov
3 years ago

  • Keywords commit added

It looks like the issue comes from this line in wp-admin/post.php:

$post = get_post( $post_id, OBJECT, 'edit' );

The ID, post_parent, and menu_order properties of the WP_Post class are documented as integers, so I thought it would be a safe assumption to always treat them as such. However, that is not the case when get_post() is called with an edit, attribute, or js context, because all values are run through esc_attr() or esc_js() in that case, and these properties are unexpectedly converted to strings.

I believe we should fix that and make the type of these properties consistent in all contexts, that should also help avoid similar issues elsewhere in the future. See 52995.diff.

#8 @SergeyBiryukov
3 years ago

  • Keywords commit removed
  • Resolution set to fixed
  • Status changed from reopened to closed

On second thought, this applies not only to WP_Post, but also to WP_Term and WP_User.

Moving the patch to a new ticket: #53235, re-closing this one as fixed. Thanks again!

#9 @SergeyBiryukov
3 years ago

In 50935:

General: Ensure consistent type for integer properties of WP_Post, WP_Term, and WP_User.

Previously, these properties could be unexpectedly converted to strings in some contexts.

This applies to the following functions:

  • sanitize_post_field()
  • sanitize_term_field()
  • sanitize_user_field()

and the following properties:

  • WP_Post::ID
  • WP_Post::post_parent
  • WP_Post::menu_order
  • WP_Term::term_id
  • WP_Term::term_taxonomy_id
  • WP_Term::parent
  • WP_Term::count
  • WP_Term::term_group
  • WP_User::ID

Props grantmkin, SergeyBiryukov.
Fixes #53235. See #52995.

Note: See TracTickets for help on using tickets.