Make WordPress Core

Opened 7 months ago

Closed 5 months ago

Last modified 5 months ago

#52995 closed defect (bug) (fixed)

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

Reported by: ravipatel Owned by: 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 7 months ago.
undefine variable
52995.diff (1.8 KB) - added by SergeyBiryukov 5 months ago.

Download all attachments as: .zip

Change History (11)

7 months ago

undefine variable

#1 @davidbaumwald
7 months 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
7 months 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
7 months 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
7 months 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
5 months 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
5 months 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.

#7 @SergeyBiryukov
5 months 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
5 months 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
5 months 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.