WordPress.org

Make WordPress Core

Opened 10 months ago

Closed 10 months ago

Last modified 10 months ago

#45711 closed defect (bug) (fixed)

Page listing is no longer hierarchical

Reported by: david.binda Owned by: desrosj
Milestone: 5.0.2 Priority: normal
Severity: normal Version: 5.1
Component: Posts, Post Types Keywords: has-patch
Focuses: administration Cc:
PR Number:

Description (last modified by SergeyBiryukov)

With [44185] being in place, the listing of pages on wp-admin/edit.php?post_type=page is no longer hierarchical, because the !isset($orderby) part of the condition in https://core.trac.wordpress.org/browser/branches/5.0/src/wp-admin/includes/post.php?rev=44185#L1089 is no longer true.

Attachments (1)

45711.diff (665 bytes) - added by desrosj 10 months ago.

Download all attachments as: .zip

Change History (12)

This ticket was mentioned in Slack in #core by pento. View the logs.


10 months ago

#2 @SergeyBiryukov
10 months ago

  • Component changed from General to Posts, Post Types
  • Focuses administration added
  • Milestone changed from Awaiting Review to 5.0.2

#3 @SergeyBiryukov
10 months ago

  • Description modified (diff)

#4 @TimothyBlynJacobs
10 months ago

Instead of making the default values empty strings, what about making them null. That seems closer to what the expected behavior would be, short of conditionally building the compact list of variables.

#5 @desrosj
10 months ago

  • Keywords has-patch added

Thanks for reporting this, @david.binda!

I think changing this to empty() is OK. There does not seem to be any specific reasoning for using ! isset() over empty()`. I traced this down to [15491].

@TimothyBlynJacobs I plan to open a ticket that documents all uses of compact() in core in order explore removing it (similar to how extract() was removed from core). I am hoping we can explore and establish a better pattern then.

@desrosj
10 months ago

#6 @desrosj
10 months ago

  • Keywords dev-feedback added

Went through the other compact() related changes to make sure no other similar situations are happening.

  • [43832] should not be affected. Setting to an empty string will result in the same behavior as if the variables were not set.
  • [43819] also does not seem to have a similar issue.
  • Other instances in [44185] should not cause issues.

#7 @pento
10 months ago

  • Keywords commit added; dev-feedback removed
  • Owner set to desrosj
  • Status changed from new to assigned

Given that WP_Query does an empty() test on the orderby parameter (ref), I don't think there's a practical difference between the orderby parameter not being set, and it being set to an empty value.

Core doesn't call wp_edit_posts_query() with the orderby parameter set to an empty value, so there's no difference to core cause by 45711.diff.

I've had a quick look through the plugins directory, I haven't found any wp_edit_posts_query() calls that would be affected by this change.

45711.diff is 👍🏻 to commit.

#8 @desrosj
10 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 44338:

Posts, Post Types: Correctly show hierarchical post type hierarchy in admin.

In [44185], a bug was introduced where hierarchical post types would not display in the correct default order (hierarchically).

This was caused by a ! isset() check, which returned false after [44185], causing the correct default value to not be applied. This switches that conditional to use an empty() check, ignoring the new empty string assignment that was added to prevent a PHP notice when compact() is called.

Props davidbinda.
Fixes #45711.

#9 @desrosj
10 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening to backport.

#10 @desrosj
10 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 44339:

Posts, Post Types: Correctly show hierarchical post type hierarchy in admin.

In [44185], a bug was introduced where hierarchical post types would not display in the correct default order (hierarchically).

This was caused by a ! isset() check, which returned false after [44185], causing the correct default value to not be applied. This switches that conditional to use an empty() check, ignoring the new empty string assignment that was added to prevent a PHP notice when compact() is called.

Merges [44338] to the 5.0 branch.

Props davidbinda.
Fixes #45711.

#11 @desrosj
10 months ago

  • Keywords commit removed
Note: See TracTickets for help on using tickets.