WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#18722 closed defect (bug) (fixed)

Missing link to edit term objects of non-public taxonomy

Reported by: johnbillion Owned by: duck_
Milestone: 3.3 Priority: normal
Severity: normal Version: 3.1
Component: Taxonomy Keywords: has-patch
Focuses: Cc:

Description

Related to #18706. On the taxonomy screen for a non-public taxonomy, links to the posts belonging to each term are not present due to erroneous use of the taxonomy's 'public' attribute.

Attachments (6)

18722.patch (425 bytes) - added by johnbillion 4 years ago.
18722.2.diff (975 bytes) - added by duck_ 3 years ago.
18722.check-ptype_object.diff (564 bytes) - added by duck_ 3 years ago.
18722.use-global.diff (627 bytes) - added by duck_ 3 years ago.
18722.no-global.1.diff (1.2 KB) - added by coffee2code 3 years ago.
Example of making potentially restrictive if() fully permissive
18722.no-global.2.diff (2.9 KB) - added by coffee2code 3 years ago.
Patch to help screen figure out its post_type

Download all attachments as: .zip

Change History (17)

@johnbillion4 years ago

comment:1 @johnbillion4 years ago

  • Keywords has-patch added

Patch - however this only works in conjunction with Sergey's patch on #18706. Should probably have merged them. Oh well.

comment:2 @SergeyBiryukov4 years ago

  • Milestone changed from Awaiting Review to 3.3

@duck_3 years ago

comment:3 follow-up: @duck_3 years ago

  • Version changed from 3.3 to 3.1

From IRC:

I agree with "erroneous use of the taxonomy's 'public' attribute", but don't know why we would check the taxonomy's show_ui for a link to a list of posts (or other content). Surely we would want to check if the post_type has show_ui => true?

See 18722.2.diff. Also includes removal of $post_type global and only adds $post_type to $args if != 'post'.

comment:4 @duck_3 years ago

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

In [19383]:

Link the posts count on taxonomy list table for non-public taxonomies. Instead check if the post type has show_ui => true as we're linking to the post list table. Fixes #18722.

comment:5 @nacin3 years ago

In [19385]:

Remove $post_type global usage from media list table. Correct $post_type to 'attachment' for upload screens. see #18722.

comment:6 in reply to: ↑ 3 @duck_3 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Replying to duck_:

Also includes removal of $post_type global

This causes a notice when adding new terms as get_current_screen()->post_type returns an empty string during the add-tag AJAX request, so get_post_type_object() returns null.

A check should be added before accessing ->show_ui, and we might want to go back to $post_type global too if the post count should be linked even for new terms with zero posts.

comment:7 follow-up: @nacin3 years ago

What about edit tag? Let's just revert that.

@duck_3 years ago

comment:8 in reply to: ↑ 7 @duck_3 years ago

Replying to nacin:

What about edit tag? Let's just revert that.

Good point, see 18722.use-global.diff.

comment:9 @duck_3 years ago

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

In [19415]:

Go back to $post_type global as get_current_screen()->post_type is empty during add/edit-tag AJAX requests. Fixes #18722.

@coffee2code3 years ago

Example of making potentially restrictive if() fully permissive

@coffee2code3 years ago

Patch to help screen figure out its post_type

comment:10 @coffee2code3 years ago

I think what @duck_ had there previously could be used with minor modification and without needing to revert to using the $post_type global again.

Is this line too restrictive? It's why the post_type couldn't be properly inferred. Currently, it only attempts to consider $_REQUEST['post_type'] for the post_type when WP_Screen::get() is called without an argument, which is essentially when set_current_screen() is called without an argument (i.e. for the current screen).

In related AJAX calls, set_current_screen() is called with an argument explicitly indicating the screen context. As such, $hook_name has a value and $_REQUEST is thus never consulted even though doing so would correctly set the post_type. In the aforementioned line, simply setting it to if ( true ) (or removing the if() from around the code it wraps) seems to work without notices. See 18722.no-global.1.diff for simple, minimal-change example (not intended as final patch).

So regardless of the value of $hook_name in WP_Screen::get(), couldn't we continue with further checking $_REQUEST if $post_type isn't yet set? See patch 18722.no-global.2.diff which:

  • Removes use of global $post_type reintroduced in [19415]
  • Consults $_REQUEST only if $post_type or $taxonomy aren't set
  • Flattens two switch() blocks into one

Perhaps better filed as a new enhancement ticket for 3.4?

comment:11 @nacin3 years ago

The issue is that WP_Screen is not necessarily the current screen. Only if we are relying on the global hook_suffix do we know that we can actually glean something off of the URL. Otherwise it may be a convert_to_screen() call for a meta box (for example) that doesn't even belong on that page.

I've thought about ways to tweak the architecture of WP_Screen to allow us to indicate when we *do* want to calculate things off $_REQUEST, because of the current order of operations prevents us from knowing at WP_Screen::get() time that we want it for the current screen. We had a usage of set_current_screen( $hook_name ) in an ajax request but found we could get rid of it, rather than fixing this root problem.

I think what we need is WP_Screen::ajax( $hook_name ), which will use ::get()'s innards. Definitely 3.4.

Note: See TracTickets for help on using tickets.