Screen Options on taxonomy edit screen

[milindmore22]

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

[milindmore22]

Attached patch to remove Screen Options from taxonomy edit screen

[swissspidy]

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.

[swissspidy]

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]

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.

[swissspidy]

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

[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 ?

[swissspidy]

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.

[seanchayes]

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​

[swissspidy]

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.

[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? :)

[swissspidy]

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​

[swissspidy]

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

[boonebgorges]

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.

[swissspidy]

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

[Ipstenu]

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

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

[boonebgorges]

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.)

[Ipstenu]

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]

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?

[swissspidy]

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.

[swissspidy]

In 36309:

Taxonomy: Fix unit tests after [36308].

See #34988.

[danielbachhuber]

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

[paulwilde]

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

[swissspidy]

@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.

[johnbillion]

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

[danielbachhuber]

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.

[DrewAPicture]

In 36497:

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

See #34988. See #33701.

[DrewAPicture]

In 36498:

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

See #34988. See #33701.

[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.

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

[danielbachhuber]

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

[swissspidy]

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?

[danielbachhuber]

Replying to swissspidy:

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

Why?

[ocean90]

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.).

[swissspidy]

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.

[swissspidy]

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?

[Chouby]

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.

[swissspidy]

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]

Fixing the unit tests

[swissspidy]

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.

[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.

[swissspidy]

Fix tag_ID variable in edit-tag-form.php

[swissspidy]

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?

[Chouby]

The new patch looks good for me.

[swissspidy]

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.

[swissspidy]

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.

[swissspidy]

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.

[ocean90]

34988-bc-fix.diff​ looks good.

[boonebgorges]

Replying to ocean90:

34988-bc-fix.diff​ looks good.

Ditto.

[swissspidy]

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.

[ryan]

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?

[ryan]

Edit term, after. No more screen options.

[ryan]

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

[szaqal21]

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

[swissspidy]

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

[szaqal21]

Replying to swissspidy:

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

Created 36893.