WordPress.org

Make WordPress Core

Opened 8 weeks ago

Last modified 4 weeks ago

#54521 new defect (bug)

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

Reported by: panagiotis.synetos Owned by:
Milestone: 6.0 Priority: normal
Severity: normal Version: 5.8.2
Component: Taxonomy Keywords: has-patch
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 8 weeks ago.
54521-taxonomy-term-quick-edit.diff (1.5 KB) - added by panagiotis.synetos 5 weeks ago.
54521-taxonomy-term-quick-edit.2.diff (1.0 KB) - added by panagiotis.synetos 5 weeks ago.

Download all attachments as: .zip

Change History (7)

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


5 weeks ago

  • 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
5 weeks 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
5 weeks 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
4 weeks 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 ideas on how to move forward? Obviously we cannot remove sanitization.

Thanks and have a good one

Last edited 4 weeks ago by panagiotis.synetos (previous) (diff)
Note: See TracTickets for help on using tickets.