Make WordPress Core

Opened 2 years ago

Last modified 19 months ago

#54521 new defect (bug)

Taxonomy term quick edit does not save if taxonomy has non latin characters

Reported by: panagiotissynetos's profile panagiotis.synetos Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 5.8.2
Component: Taxonomy Keywords: has-patch has-unit-tests dev-feedback
Focuses: administration Cc:

Description

This issue started from the following WooCommerce ticket

https://github.com/woocommerce/woocommerce/issues/31037

After investigating, I could reproduce the issue by doing the following

  • Register a new taxonomy that contains Greek character wp_ελληνικό_tax
  • register_taxonomy( 'wp_ελληνικό_tax', array( 'post' ), $args );
    
  • Create a term
  • Edit the term through quick edit

https://user-images.githubusercontent.com/2484390/141967192-60de334d-e5ec-437e-9574-3cd9aa30110c.png

(for a strange reason, I cannot make the image appear inline)

  • You'll get a 0 error, with no indication
  • If you edit the term through the normal edit (not quick edit) it works fine.

I already have a suggestion on how to fix this error

https://developer.wordpress.org/reference/functions/wp_ajax_inline_save_tax/

This wp_ajax_inline_save_tax function, sanitizes the taxonomy, and wp_ελληνικό_tax becomes wp__tax, which doesn't exist. (hence the 0 error)

Why is the taxonomy needed? Cause currently, it calls get_term , passing term_id and taxonomy_slug.

Instead, what we could do is

  • avoid sanitizing the taxonomy slug, seems like there is no need
  • try and get the tag/term earlier, by calling get_term with just the term_id (as term_id is unique, regardless of the taxonomy it belongs to)

I have attached a revised version of this function

<?php

function wp_ajax_inline_save_tax() {
        check_ajax_referer( 'taxinlineeditnonce', '_inline_edit' );

        if ( ! isset( $_POST['tax_ID'] ) || ! (int) $_POST['tax_ID'] ) {
                wp_die( -1 );
        }

        $id = (int) $_POST['tax_ID'];

        if ( ! current_user_can( 'edit_term', $id ) ) {
                wp_die( -1 );
        }

        // Try and get the tag just by ID, without the taxonomy argument
        $tag = get_term( $id );
        if ( null === $tag || is_wp_error( $tag ) ) {
                wp_die( 0 );
        }

        $taxonomy = $tag->taxonomy;

        $wp_list_table = _get_list_table( 'WP_Terms_List_Table', array( 'screen' => 'edit-' . $taxonomy ) );

//      $tag                  = get_term( $id, $taxonomy );
        $_POST['description'] = $tag->description;
........
.......

}

If agreed, I'd like to work on this issue and do my first contribution on WordPress.

Panos Synetos
Code Wrangler @ Automattic

Attachments (3)

141967192-60de334d-e5ec-437e-9574-3cd9aa30110c.png (534.5 KB) - added by panagiotis.synetos 2 years ago.
54521-taxonomy-term-quick-edit.diff (1.5 KB) - added by panagiotis.synetos 2 years ago.
54521-taxonomy-term-quick-edit.2.diff (1.0 KB) - added by panagiotis.synetos 2 years ago.

Download all attachments as: .zip

Change History (12)

This ticket was mentioned in PR #2057 on WordPress/wordpress-develop by PanosSynetos.


2 years ago
#1

  • Keywords has-patch added

WordPress allows taxonomies to be created with non-latin characters.

Quick edit on terms that belong to those taxonomies, fail with an error 0.

The issue started from this https://github.com/woocommerce/woocommerce/issues/31037 ticket.

Trac ticket: https://core.trac.wordpress.org/ticket/54521

#2 @SergeyBiryukov
2 years ago

  • Milestone changed from Awaiting Review to 6.0

Hi there, welcome to WordPress Trac! Thanks for the ticket and the patch.

This seems like a remnant from before WordPress 4.4, when the $taxonomy argument was still required. It was later made optional in [34997] / #14162 as part of a larger taxonomy overhaul that started in WordPress 4.1:

The patch looks good at a glance. Would be great to include a unit test for this, phpunit/tests/ajax/QuickEdit.php can probably be used as a starting point.

#3 @panagiotis.synetos
2 years ago

Hey, thanks @SergeyBiryukov for the welcome and the suggestions. I thought that since this function is there for ages, that it should have a unit test, that's why I didn't create one.

I'll go ahead and create one early next week and submit it.

Thanks, and when you get a chance, could you please assign me this ticket? :)

Have a good one

#4 @panagiotis.synetos
2 years ago

Fixing one thing, causes another to break @SergeyBiryukov , which I found while writing the Unit Test.

I'll try and come back with more info before the end of the week if needed, but long story short

Inside class WP_Screen https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-admin/includes/class-wp-screen.php#L250 there is a sanitize happening on the ID (which on our case, it's the taxonomy slug)

Sanitizing something that is called wp_ελληνικό_tax gets converted to wp__tax and hell breaks loose down the line, https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-admin/includes/class-wp-terms-list-table.php#L572 , as $this->screen->taxonomy is empty, same for the post_type , ending up with this notice

(I just run phpunit --group ajax --filter Tests_Ajax_QuickEdit so I don't have to wait for the whole suite to run when npm run test:php )

..E                                                                 3 / 3 (100%)

Time: 00:01.547, Memory: 172.50 MB

There was 1 error:

1) Tests_Ajax_QuickEdit::test_term_quick_edit_non_latin_slug
Trying to get property 'show_ui' of non-object

/var/www/src/wp-admin/includes/class-wp-terms-list-table.php:585
/var/www/src/wp-admin/includes/class-wp-list-table.php:1445
/var/www/src/wp-admin/includes/class-wp-terms-list-table.php:345
/var/www/src/wp-admin/includes/ajax-actions.php:2146
/var/www/src/wp-includes/class-wp-hook.php:303
/var/www/src/wp-includes/class-wp-hook.php:327
/var/www/src/wp-includes/plugin.php:470
/var/www/tests/phpunit/includes/testcase-ajax.php:265
/var/www/tests/phpunit/tests/ajax/QuickEdit.php:150

I've commit the unit tests, demonstrating that everything works fine with taxonomies with latin characters and that it fails with non-latin/accented.

Any suggestions on how to move forward?

Thanks and have a good one

Version 0, edited 2 years ago by panagiotis.synetos (next)

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


2 years ago

#6 @sumitsingh
2 years ago

  • Keywords has-unit-tests dev-feedback added

#7 @costdev
2 years ago

  • Milestone changed from 6.0 to 6.1

As this ticket still needs more investigation and today is 6.0 RC1, I'm moving this ticket to the 6.1 milestone.

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


19 months ago

#9 @audrasjb
19 months ago

  • Milestone changed from 6.1 to Future Release

Unfortunately, this ticket didn’t get more momentum in 6.1 than in 6.0… sorry about that, but the proposed patch still needs testing, so let's move it to Future Release milestone, at least until it's properly reviewed and tested.

Note: See TracTickets for help on using tickets.