WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 20 months ago

Last modified 18 months ago

#34988 closed defect (bug) (fixed)

Screen Options on taxonomy edit screen

Reported by: milindmore22 Owned by: swissspidy
Milestone: 4.5 Priority: normal
Severity: normal Version: 4.4
Component: Taxonomy Keywords: has-patch commit
Focuses: administration Cc:

Description

Hello,

I been thinking why we need Screen Options on taxonomy edit screen( edit-tags.php).

Those screen options are for taxonomy listing, where user can choose to show/hide columns in listing they are no use on taxonomy edit screen

http://milind.rtcamp.net/wp-content/uploads/2015/12/Edit-Category-‹-example.com-—-WordPress-Google-Chrome_007.png

Attachments (17)

34988.patch (1.3 KB) - added by milindmore22 2 years ago.
Attached patch to remove Screen Options from taxonomy edit screen
34988.2.patch (1.1 KB) - added by swissspidy 2 years ago.
34988.3.patch (12.8 KB) - added by swissspidy 2 years ago.
34988.4.patch (12.1 KB) - added by seanchayes 2 years ago.
34988.5.patch (13.0 KB) - added by swissspidy 2 years ago.
34988.6.patch (13.3 KB) - added by swissspidy 2 years ago.
34988.diff (5.8 KB) - added by swissspidy 23 months ago.
adminbar.diff (534 bytes) - added by swissspidy 22 months ago.
34988.2.diff (526 bytes) - added by paulwilde 22 months ago.
34988.3.diff (1.8 KB) - added by swissspidy 21 months ago.
34988.4.diff (2.9 KB) - added by swissspidy 21 months ago.
34988.5.diff (4.1 KB) - added by swissspidy 21 months ago.
Fixing the unit tests
34988.6.diff (584 bytes) - added by Chouby 21 months ago.
34988-tag_ID.diff (1.7 KB) - added by swissspidy 21 months ago.
Fix tag_ID variable in edit-tag-form.php
34988-bc-fix.diff (401 bytes) - added by swissspidy 20 months ago.
edit-term-after.png (493.5 KB) - added by ryan 20 months ago.
Edit term, after. No more screen options.
edit-term-before.png (479.5 KB) - added by ryan 20 months ago.
edit term, before. With screen options carried over from the list table.

Download all attachments as: .zip

Change History (78)

@milindmore22
2 years ago

Attached patch to remove Screen Options from taxonomy edit screen

@swissspidy
2 years ago

#1 @swissspidy
2 years ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 4.5

This shouldn't really be handled in WP_Screen class, at least not in WP_Screen::show_screen_options. Not really responsible for that and makes it harder for others to add screen options.

34988.2.patch is another patch, which simply doesn't add screen options for that screen.

However, the real issue IMHO is that the term edit screen isn't in its own file, which means a list table is added without reason. For example, the posts list table is in wp-admin/edit.php, the edit post screen is in wp-admin/post.php, while here everything is in edit-tags.php.

I'd rather see us introduce a new term.php file for editing a single term, if it can be done without breaking too much things.

#2 @swissspidy
2 years ago

  • Keywords dev-feedback added

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


2 years ago

#4 @swissspidy
2 years ago

As discussed in the chat linked above, we should keep this simple but eventually try to add a new term.php edit page to separate logic between list view and single edit view.

@swissspidy
2 years ago

#5 @swissspidy
2 years ago

  • Keywords needs-testing added

As discussed in the chat linked above, we should keep this simple but eventually try to add a new term.php edit page to separate logic between list view and single edit view.

Well, I just played with it and it is easier than I thought. 34988.3.patch separates the logic. Editing a term works flawlessly. I added a redirect from the old place to catch any old URLs, so that'd be safe.

This patch of course also fixes the initially reported issue.

#6 @swissspidy
2 years ago

Maybe edit-tag-form.php could also be incorporated into term.php? It's not used anywhere, but I'm not sure about BC.

#7 follow-up: @milindmore22
2 years ago

@swissspidy

Loved the idea of term.php while testing your patch I noticed since user is landing on term.php for term editing If user directly visits page it say Invalid Taxonomy it should be Invalid term right ?

#8 in reply to: ↑ 7 @swissspidy
2 years ago

Replying to milindmore22:

@swissspidy

Loved the idea of term.php while testing your patch I noticed since user is landing on term.php for term editing If user directly visits page it say Invalid Taxonomy it should be Invalid term right ?

You're right. I'll update the patch to move the $term = get_term( $term_id, $taxonomy, OBJECT, 'edit' ); check up. There's no need for a separate taxonomy check.

#9 @seanchayes
2 years ago

I took a look at this patch to test as well and:

  • refreshed the patch to restore the change to edit-tag-form.php
  • reworded the 2nd wp_die message to say "term" in term.php
  • adjusted a variable name used in term.php

The code functionality worked as expected as well as addressing the original ticked issue to not display "Show Options" in the screen area.

As an aside, during my testing, I found that a couple of plugins seemed to be encoding the "&" in the action row links when passing term_id to the new term.php. So, for example, on a custom tag based taxonomy the url was:

...blah/term.php?taxonomy=custom&wp_http_referer=%2Fwp-admin%2Fedit-tags.php%3Ftaxonomy%3Dcustom%26post_type%3Dcpt#038;term_id=48&post_type=cpt

That led to a "You did not select an item for editing." message.

I did not encounter that behavior with the Post category or Post tag.

Patch attachment:34988.4.patch

@seanchayes
2 years ago

@swissspidy
2 years ago

#10 follow-up: @swissspidy
2 years ago

From my previous comment:

I'll update the patch to move the $term = get_term( $term_id, $taxonomy, OBJECT, 'edit' ); check up. There's no need for a separate taxonomy check.

The whole term.php can be simplified even further:

  • Passing a taxonomy is not really necessary because there are no shared terms anymore.
  • Passing the post type can be optional, because we can always default to the first object_type associated with the taxonomy if no post type was passed.

Because of that, [35875] is kinda made obsolete here, because we need to check for the term before loading the admin header. Pinging @afercia for feedback :)

Note that in the previous patches, the <title> was broken. My new patch 34988.5.patch fixes this by correctly setting $title.

#11 in reply to: ↑ 10 ; follow-up: @afercia
2 years ago

Replying to swissspidy:

Because of that, [35875] is kinda made obsolete here, because we need to check for the term before loading the admin header. Pinging @afercia for feedback :)

So if there's no tag ID, it just dies and doesn't get redirected, correct? :)

@swissspidy
2 years ago

#12 in reply to: ↑ 11 @swissspidy
2 years ago

Replying to afercia:

Replying to swissspidy:

Because of that, [35875] is kinda made obsolete here, because we need to check for the term before loading the admin header. Pinging @afercia for feedback :)

So if there's no tag ID, it just dies and doesn't get redirected, correct? :)

Correct. However, I thought I should make it more similar to how post.php handles it:

  • Die when an invalid term is passed.
  • Redirect to edit-tags.php if no term is passed.

See 34988.6.patch

#13 @afercia
23 months ago

  • Owner set to swissspidy
  • Status changed from new to assigned

#14 @swissspidy
23 months ago

  • Keywords commit added; dev-feedback needs-testing removed

I'd like to get this in early in the cycle so we have time to react if something would break.

#15 @boonebgorges
23 months ago

term.php seems like a pretty good idea.

You've changed $tag to $term throughout. $term is, of course, more accurate. But this variable exists in the global scope, so it's possible (likely, knowing WP plugins) that people are expecting to be able to find the current term at global $tag. It might be worth canvassing the plugin repo before deciding whether to change the variable name.

#16 @swissspidy
23 months ago

Does someone have a local copy of the plugin directory by chance? A quick Google search on the GitHub mirror showed 0 results.

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


23 months ago

#18 @Ipstenu
23 months ago

Found nineteen (19) including BuddyPress ... :|

See https://cloudup.com/cVp_EmttbAN for all of them.

#19 @boonebgorges
23 months ago

Thanks, @Ipstenu !

Of those in your document:

  • BuddyPress is a false positive. The affected file is in the packaged bbPress, which is not used by default anymore, and is part of an AJAX handler that is never called in the BP context in any case.
  • evergage is a false positive. Appears to fire only on the front end, when global $tag shouldn't be populated (right?)
  • I'm pretty sure lastfm-recent-tracks is a false positive - it's using $tag to pass stuff around between XML-formatting callbacks. Same with ml-social, mlv-contextual
  • socialmarketing invokes the global but never uses it. Same with wp-analitify

My impression from ten seconds looking at each is that only about five or six will out-and-out break if this change is made.

I'm probably overly conservative, but given the limited upside of making the change, I'd leave it as $tag. (Another option: $GLOBALS['tag'] =& $term. Shudder.)

#20 @Ipstenu
23 months ago

FWIW some of them call it multiple times. I had it set to only flag the first call. I doubt this will change anything but here's a fuller report.

https://cloudup.com/ctwHXdYq0oq

@swissspidy
23 months ago

#21 @swissspidy
23 months ago

Thanks to both of you for looking into this! 34988.diff is an updated patch undoing the variable renaming. Still works like a charm, but now we're more safe.

Looks good?

#22 @swissspidy
23 months ago

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

In 36308:

Taxonomy: Introduce wp-admin/term.php for editing single terms.

This is similar to edit.php -> post.php and users.php -> user-edit.php and fixes a bug where screen options for the list table were shown while editing a term.

Fixes #34988.

#23 @swissspidy
23 months ago

In 36309:

Taxonomy: Fix unit tests after [36308].

See #34988.

#24 @danielbachhuber
22 months ago

Won't this break any conditionals based on $pagenow?

#25 @paulwilde
22 months ago

This change has removed the "View Taxonomy Name" link in the admin bar when on the edit page.

Last edited 22 months ago by paulwilde (previous) (diff)

#26 @paulwilde
22 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

@paulwilde
22 months ago

#27 @swissspidy
22 months ago

  • Keywords commit removed

@paulwilde Thanks for the hint! Looks like we were equally fast with the patches :-)

I guess that's what @danielbachhuber meant in the comment above, although changing $pagenow or the current screen name back to edit-tags feels wrong.

#28 @johnbillion
22 months ago

Is there a back-compat issue here with plugins which are expecting term editing to take place on edit-tags.php?

#29 @johnbillion
22 months ago

  • Keywords dev-feedback added

#30 @danielbachhuber
22 months ago

While I'm sympathetic to what's been committed as an improvement, I suspect there are moderate to severe back-compat issues with introducing an entirely new page.

#31 @DrewAPicture
22 months ago

In 36497:

Docs: Add a missing version to the file header for wp-admin/term.php, introduced in [36308].

See #34988. See #33701.

#32 @DrewAPicture
22 months ago

In 36498:

Docs: Revert unintended changes in wp-includes/post.php, mistakenly included in [36497].

See #34988. See #33701.

#33 @johnbillion
21 months ago

  • Keywords needs-patch added; has-patch removed

#34 @atimmer
21 months ago

This will break any plugin that adds something to the default term edit and checks for that using edit-tags.php. Also because the query parameter for the ID has been changed from tag_ID to term_id on term.php, any plugin that assumes tag_ID contains the term ID will error.

I think we need at least a post on make/core explaining this change and possible impact for plugin authors.

See: https://github.com/Yoast/wordpress-seo/issues/3944

#35 @danielbachhuber
21 months ago

@swissspidy given the backwards compat problems, are we going to revert this?

#36 follow-ups: @swissspidy
21 months ago

Thanks for the feedback everyone. I only now found time to have another look at this.

I really think that adding term.php is the right choice, either now or in a later release. Since [36308] there haven't been any reports of breakage except for the one in Yoast SEO.

I'd like to better understand how plugins are relying on the existence of edit-tags.php / the edit-tags screen name. A make post on the topic is a great idea and is definitely something to be considered. If this is not enough, we could also adjust the global variables in term.php instead of reverting. At least for this release.

It's not something I want to decide alone, though.

@atimmer Out of curiosity, why are you fetching the term ID from the query parameter instead of the global variable? And how difficult would it be to fix the plugin?

#37 in reply to: ↑ 36 ; follow-up: @danielbachhuber
21 months ago

Replying to swissspidy:

I really think that adding term.php is the right choice, either now or in a later release.

Why?

#38 @ocean90
21 months ago

Having a separate page to edit terms seems fine, it would match the structure of the Post UI, edit.php + post.php.

Before asking for a revert you should try to provide some sort of back-compat. So far it looks like it's only $pagenow and the query var, both can be easily restored.
It's not really a big deal if scripts or other stuff is loaded on the both pages because edit-tags.php includes the form for adding new terms (which is different from Post UI.).

#39 in reply to: ↑ 37 @swissspidy
21 months ago

Replying to danielbachhuber:

Replying to swissspidy:

I really think that adding term.php is the right choice, either now or in a later release.

Why?

Not only does it make the admin more consistent by matching the structure of edit.php/post.php and users.php/user-edit.php, it also clearly separates list view and edit view, preventing such weird errors like the one mentioned here. It's just more future-proof.

I'll work on a new patch fixing $pagenow, the query var, and the admin bar.

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


21 months ago

@swissspidy
21 months ago

#41 @swissspidy
21 months ago

34988.3.diff resets $pagenow, the tag_ID query var and the action query var. With that, the mentioned plugin still works like expected, though I have to say it really should not rely on the these query vars but on the global variables instead.

If we "fix" $pagenow now, could we ever change that to term.php?

#42 in reply to: ↑ 36 @Chouby
21 months ago

Replying to atimmer:

This will break any plugin that adds something to the default term edit and checks for that using edit-tags.php. Also because the query parameter for the ID has been changed from tag_ID to term_id on term.php, any plugin that assumes tag_ID contains the term ID will error.

Replying to swissspidy:

I'd like to better understand how plugins are relying on the existence of edit-tags.php / the edit-tags screen name. A make post on the topic is a great idea and is definitely something to be considered.

I am currently testing WP 4.5 beta1 and ran into issues related to this ticket with Polylang as I rely on $_GET['tag_ID'] to get the term_id of the current term being edited and on $screen->base to decide whether to enqueue a script or not.

In both cases, the fix is trivial. Thus, for me, there is no reason to revert. A make post and/or direct e-mail contact with authors of plugins which may break due to the lack of backward compatibility (I received such e-mail in the past) should be ok to keep things as they are.

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


21 months ago

@swissspidy
21 months ago

#44 @swissspidy
21 months ago

  • Keywords has-patch commit added; needs-patch removed

After taking a closer look at this together with @ocean90, 34988.4.diff is a new patch incorporating the feedback we got over the last few weeks. In detail:

  • Runs do_action( 'edit-tags.php' ); on the term edit page for backward compatibility
  • Changes the GET param back to tag_ID
  • Improve compatibility in WP_Screen to properly set the screen base and globals like $taxnow on the new term edit page.
  • Also fixes the "View Taxonomy Name" link in the toolbar (props paulwilde).

So the only bigger change will be the one of $screen->base from edit-tags to term for the new page.

After committing this there will be a post on make/core outlining the changes.

@swissspidy
21 months ago

Fixing the unit tests

#45 @swissspidy
21 months ago

In 36874:

Taxonomy: Improve backward compatibility on the wp-admin/term.php page.

Specifically, run do_action( 'edit-tags.php' ); on this new term edit page introduced in [36308]. Changes the GET param back to tag_ID and properly sets the screen base in WP_Screen.

See #34988.

#46 @swissspidy
21 months ago

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

@Chouby
21 months ago

#47 follow-up: @Chouby
21 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

@swissspidy

I'd like to reopen this ticket as I believe that the revert from the query var term_id to tag_ID was not complete. See 34988.6.patch.

Also I noticed that your make post did not mention that the variable $tag_ID has been removed and replaced by $term_id. I guess that this could impact a few plugins.

@swissspidy
21 months ago

Fix tag_ID variable in edit-tag-form.php

#48 in reply to: ↑ 47 @swissspidy
21 months ago

Replying to Chouby:

@swissspidy

I'd like to reopen this ticket as I believe that the revert from the query var term_id to tag_ID was not complete. See 34988.6.patch.

Also I noticed that your make post did not mention that the variable $tag_ID has been removed and replaced by $term_id. I guess that this could impact a few plugins.

You're right, my bad!

It wasn't mentioned in the post because that change should really have been reverted as part of [36874].

34988-tag_ID.diff should fix this. Would you mind giving that patch a spin?

#49 @Chouby
21 months ago

The new patch looks good for me.

#50 @swissspidy
21 months ago

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

In 36969:

Taxonomy: After [36874], rename $term_id to $tag_ID in wp-admin/edit-tag-form.php.

This ensures that no variables changed in the process of introducing wp-admin/term.php, improving overall backward compatibility.

Props Chouby for initial patch.
Fixes #34988.

#51 @swissspidy
20 months ago

  • Keywords needs-patch added; dev-feedback has-patch commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

I hate reopening tickets, but as pointed out in a comment on the make post, [36874] contained a bad mistake.

On the term edit page, the load-edit-tags.php hook should be called for backwards compatibility. Instead, it calls edit-tags.php, which is just wrong.

#52 @swissspidy
20 months ago

  • Keywords has-patch added; needs-patch removed

34988-bc-fix.diff includes a fix for the misspelled load-edit-tags.php hook name in wp-admin/admin.php (see [36874]). Since this was already communicated on make/core it's important that this code actually does what was mentioned in the blog post.

#53 follow-up: @ocean90
20 months ago

  • Keywords commit added

34988-bc-fix.diff looks good.

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


20 months ago

#55 in reply to: ↑ 53 @boonebgorges
20 months ago

Replying to ocean90:

34988-bc-fix.diff looks good.

Ditto.

#56 @swissspidy
20 months ago

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

In 37084:

Taxonomy: After [36874], run the correct load-edit-tags.php hook on the new term edit page.

When not misspelled, this hook is useful (and needed) for backward compatibility.

Unprops swissspidy.
Fixes #34988.

#57 @ryan
20 months ago

Cross linking make/core post.

https://make.wordpress.org/core/2016/03/07/changes-to-the-term-edit-page-in-wordpress-4-5/

I don't see screenshots on this ticket or on the post. What all changed here? I see that Screen Options no longer shows on the term edit page. Anything else?

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


20 months ago

@ryan
20 months ago

Edit term, after. No more screen options.

@ryan
20 months ago

edit term, before. With screen options carried over from the list table.

#59 @szaqal21
18 months ago

Don't know why but filter for terms list columns ('manage_edit-department_name_columns') is being still applied in term.php.

#60 follow-up: @swissspidy
18 months ago

@szaqal21 Can you create a new ticket for that? This ticket was closed on a completed milestone. Thanks!

#61 in reply to: ↑ 60 @szaqal21
18 months ago

Replying to swissspidy:

@szaqal21 Can you create a new ticket for that? This ticket was closed on a completed milestone. Thanks!

Created 36893.

Note: See TracTickets for help on using tickets.