WordPress.org

Make WordPress Core

Opened 20 months ago

Closed 16 hours ago

#36514 closed defect (bug) (fixed)

posting with custom taxes

Reported by: hokku Owned by: boonebgorges
Milestone: 5.0 Priority: normal
Severity: normal Version: 4.2
Component: Taxonomy Keywords: has-patch has-unit-tests
Focuses: administration Cc:

Description

Sorry for my bad english.

Seems there is a bug in wp-admin/includes/post.php on line 348.

Need for use

'include' => $term,

instead

'name' => $term,

Now it causes errors when saving posts within custom non-hierarchical taxonomies - WordPress by mistake spontaneously creates new items which has names like id's cheked items.

Attachments (5)

36514.diff (3.1 KB) - added by boonebgorges 19 months ago.
36514-2.diff (3.2 KB) - added by stephenharris 17 months ago.
36514.2.diff (5.3 KB) - added by boonebgorges 14 months ago.
36514.3.diff (5.8 KB) - added by boonebgorges 13 months ago.
36514.4.diff (7.2 KB) - added by ZaneMatthew 9 months ago.

Download all attachments as: .zip

Change History (33)

#1 @hokku
20 months ago

  • Severity changed from normal to major

#2 follow-up: @boonebgorges
20 months ago

  • Component changed from General to Taxonomy
  • Focuses administration added

By default, non-hierarchical taxonomies do not use a checkbox interface. I assume you are overriding this behavior with a custom meta_box_cb parameter when registering the taxonomy. Can you please share the code you're using to register the taxonomy, and any code you may be using to render the metabox (assuming it's not one of the core functions)?

#3 @SergeyBiryukov
20 months ago

  • Version changed from trunk to 4.2

The line in question was added in [31359].

#4 in reply to: ↑ 2 @hokku
20 months ago

Replying to boonebgorges:

By default, non-hierarchical taxonomies do not use a checkbox interface. I assume you are overriding this behavior with a custom meta_box_cb parameter when registering the taxonomy. Can you please share the code you're using to register the taxonomy, and any code you may be using to render the metabox (assuming it's not one of the core functions)?

Right, here is the code

        function create_custom_taxes(){
                $labels = array(
                        'name' => 'Слайдеры',
                        'singular_name' =>      'Слайдер',
                        'search_items' =>  'Слайдеры',
                        'all_items' => 'Слайдеры',
                        'parent_item' => '',
                        'parent_item_colon' => '',
                        'edit_item' => 'Редактировать слайдер',
                        'update_item' => 'Обновить слайдер',
                        'add_new_item' => 'Добавить слайдер',
                        'new_item_name' => 'Новый слайдер',
                        'menu_name' => 'Слайдеры'
                );

                register_taxonomy( 'slider', array('slide'), array(
                        'hierarchical' => false,
                        'labels' => $labels,
                        'show_ui' => true,
                        'show_admin_column' => true,
                        'query_var' => true,
                        'rewrite' => array( 'slug' => 'slider' ),
                        'meta_box_cb' => 'post_categories_meta_box',
                ));
        };
        add_action( 'init', 'create_custom_taxes' );

#5 @boonebgorges
19 months ago

  • Keywords 2nd-opinion has-patch added
  • Milestone changed from Awaiting Review to 4.6

Thanks for the details, @hokku. As I'd guessed, you're specifying post_categories_meta_box for a non-hierarchical taxonomy, which is confusing the logic introduced in [31359].

We need something more specific, which will tell edit_post() when it ought to be converting 'tax_input' values to term IDs. It's hard to think of a very elegant way to do this. 36514.diff is my best idea: a new meta_box_expect_ids param for register_taxonomy(), which defaults to true when the meta_box_cb is 'post_categories_meta_box' (either explicitly or as fallback, based on hierarchical). Then, in edit_post(), we check this value instead of is_taxonomy_hierarchical().

This technique is not a beautiful piece of software craftsmanship, but it will only ever matter for people who are providing a meta_box_cb other than the defaults from core, which is hardly anyone.

@boonebgorges
19 months ago

#6 @stephenharris
17 months ago

I found that the original patch left the array of term IDs as a strings. These are interpreted to be slugs (so creating a new term also created a term with the name matching the original term's ID).

The only difference to the original patch is the line which casts all the values in the array to integers.

This fixes the bug for me.

#7 @ocean90
17 months ago

  • Severity changed from major to normal

#8 @boonebgorges
17 months ago

  • Keywords 4.7-early added; 2nd-opinion removed
  • Milestone changed from 4.6 to Future Release

I think we missed the 4.6 deadline for this improvement, but let's do right away once 4.7 kicks off.

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


15 months ago

#10 follow-up: @helen
15 months ago

Related/possibly a duplicate: #14877

I think it'd be perfectly reasonable to get what exists working with both IDs and slugs as appropriate in this ticket and then using that to model new callbacks after. And I'd love to see that happen.

#11 in reply to: ↑ 10 ; follow-up: @boonebgorges
15 months ago

  • Keywords needs-patch 2nd-opinion added; has-patch 4.7-early removed
  • Milestone changed from Future Release to 4.7

Replying to helen:

Related/possibly a duplicate: #14877

I think it'd be perfectly reasonable to get what exists working with both IDs and slugs as appropriate in this ticket and then using that to model new callbacks after. And I'd love to see that happen.

If we're headed for a future where WP provides more than just two meta box callbacks for taxonomies, then we may want to opt for something a bit more sophisticated here than an if/else toggle. I'm thinking something like a meta_box_save_cb alongside meta_box_cb. We'll have an internal matrix that can be used to provide default mappings of meta_box_cb to meta_box_save_cb:

// register_taxonomy()
if ( null === $args['meta_box_save_cb'] ) ) {
    switch ( $args['meta_box_cb'] ) {
        case 'post_categories_meta_box' :
            $args['meta_box_save_cb'] = 'meta_box_save_cb_checkboxes';
        break;

        case 'post_tags_meta_box' :
        default:
            $args['meta_box_save_cb'] = 'meta_box_save_cb_input';
        break;

    }
}

The format of tax_input and the corresponding validation required will look a bit different for select, multiselect, radio, so they should each probably get their own meta_box_save_cb. This provides that framework. Thoughts?

#12 in reply to: ↑ 11 @helen
14 months ago

Replying to boonebgorges:

The format of tax_input and the corresponding validation required will look a bit different for select, multiselect, radio, so they should each probably get their own meta_box_save_cb. This provides that framework. Thoughts?

Ah, interesting. Do we break out much of the insert/update post process right now? I feel like the last time I looked it was one giant procedural call.

In practice, I think it really ends up as three callbacks - single (select, radio), multi (checkboxes, multi-select), and freeform input (currently tags).

#13 @boonebgorges
14 months ago

  • Keywords has-patch added; needs-patch removed

Ah, interesting. Do we break out much of the insert/update post process right now? I feel like the last time I looked it was one giant procedural call.

Yes, the way it's saved is a bit of a mess, and not easily separated from wp_insert_post() without changing the order in which things happen. We can simplify the improvement by asking for a sanitization callback instead. See 36514.2.diff for a proof-of-concept.

In practice, I think it really ends up as three callbacks - single (select, radio), multi (checkboxes, multi-select), and freeform input (currently tags).

Yes, this covers the native HTML elements, but who knows what wacky stuff people are doing inside of a meta_box_cb. In any case, the sanitize callbacks are good for more than just differentiating between the *shape* of the payload, they can also do specific validation on the *contents* of the payload. Imagine, for instance, that you allowed users to provide a comma-separated list of term IDs (not sure why you'd do this specifically, but you get the idea). A generic meta_box_sanitize_cb allows you to do whatever you want with your 'tax_input'.

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


13 months ago

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


13 months ago

#16 @stevenkword
13 months ago

  • Owner set to helen
  • Status changed from new to reviewing

#17 @boonebgorges
13 months ago

  • Owner changed from helen to boonebgorges

#18 @boonebgorges
13 months ago

  • Keywords 4.8-early added
  • Milestone changed from 4.7 to Future Release
  • Owner changed from boonebgorges to helen

Revisiting this: I think my approach is the right one, but it's an API enhancement masquerading as a bug fix, and at this point in the release cycle I don't have the stomach for more enhancements. Let's do it for 4.8, at which point I will gratefully accept any thoughts from @helen.

36514.3.diff is a refresh after the introduction of WP_Taxonomy.

#20 @adamsilverstein
9 months ago

  • Keywords needs-unit-tests added

@ZaneMatthew
9 months ago

#21 @ZaneMatthew
9 months ago

  • Keywords has-unit-tests added; needs-unit-tests removed

36514.4.diff

I've submitted a unit test for the proposed patch. The unit test failed before applying the patch, and passed once the patch was applied.

I was able to manually reproduced the issue via the WP Admin. I then created a unit test that;

  • registers a given taxonomy based on the reporters criteria,
  • creates a post and term for the newly registered taxonomy,
  • sets the current user to editor role,
  • uses edit_post() to apply the newly created term to the given post,

I verified that WordPress did create a new term with the term ID as the term, vs. adding the term to the post.

I then applied the patch, re-run the test, and the test passed.

One item to note is that to fully simulate the process of editing a post from the WP Admin, the $term_id had to be type casted as (string) $term_id.

#22 @ZaneMatthew
8 months ago

Hi @obenland looking for feedback on the unit test submitted. Also wanted to know if there's any other movement with this ticket.

#23 @adamsilverstein
7 months ago

  • Milestone changed from Future Release to 4.8

#24 @adamsilverstein
6 months ago

  • Milestone changed from 4.8 to 4.8.1

No movement on this ticket, punting to 4.8.1. Any feedback here, @helen, @boonebgorges or @obenland?

#25 @boonebgorges
5 months ago

  • Keywords 2nd-opinion 4.8-early removed
  • Milestone changed from 4.8.1 to 4.9
  • Owner changed from helen to boonebgorges

This is not viable for 4.8.1, but let's do it for 4.9. 36514.4.diff looks very close - I will review.

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


6 weeks ago

#27 @boonebgorges
6 weeks ago

  • Milestone changed from 4.9 to 5.0

Apologies for not reviewing this in time. Let's shoot for the next release.

#28 @boonebgorges
16 hours ago

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

In 42211:

Introduce meta_box_sanitize_cb taxonomy argument.

The meta_box_cb argument was introduced in [25572] to allow plugin
authors to provide a custom callback for rendering their taxonomy's meta
box on the post edit screen. However, the routine used to handle the saving
of these custom taxonomy meta boxes was not customizable, but was instead
based simply on whether the taxonomy was hierarchicaly. See [13535].

The new meta_box_sanitize_cb argument defaults to the "tag" routine for
non-hierarchical taxonomies and the "category" routine for hierarchical ones,
thereby maintaining the current default behavior. Developers can override this
when the data passed from their meta_box_cb differs.

Props boonebgorges, ZaneMatthew, stephenharris.
Fixes #36514.

Note: See TracTickets for help on using tickets.