search

helgatheviking / Radio-Buttons-for-Taxonomies

Turn any WordPress taxonomy into a list of radio buttons, which forces users to select only 1 term!
53 stars 28 forks source link

Quick editing removes all terms for radio button taxonomies #17

Closed haroldkyle closed 10 years ago

haroldkyle commented 10 years ago

When opening Quick Edit, no values are selected. Regardless of your subsequent selection, if you save the post in Quick Edit all terms for the radio button taxonomy are removed from the post. Tested on 1.6.1 and dev on WP 3.8.3. I am using custom hierarchical taxonomies for this test. I will try to debug later. Does the plugin offer support for Quick Edit?

helgatheviking commented 10 years ago

In theory, it should support quick edit, but quick edit has always been a bit janky. There are some problems with quick edit that I am trying to work out, but my local setup has gone a bit haywire.... SourceTree is bugging out on me. I think I managed to push most of the relevant changes today if you can pull the latest from the main branch (will still say v1.6.1).

I'd love if you could help debug. Another set of eyes would be a big help.

haroldkyle commented 10 years ago

I'll work on this today a little bit. Janky is a good way to describe Quick Edit in general!

helgatheviking commented 10 years ago

For sure. I had to hack the global taxonomies object to turn off the default checkbox UI and then replace it with my own. Anyway, 0805892df73bc61639c59a4df50b01973267b298 is what I am crossing my fingers will fix the save issue with quick edit and I think bulk edit should be in there now too if you want to give that a look. (hopefully it didn't get lost in the great SourceTree explosion of April.

haroldkyle commented 10 years ago

I'm able to get this to behave properly in a few circumstances, and so have narrowed down the reason this is happening. I can commit changes this evening that should fix this issue. "I'm on it."

haroldkyle commented 10 years ago

Just to clarify what's going on, the circumstances that I see this happening in:

OK, I should have a commit later tonight.

helgatheviking commented 10 years ago

If show_admin_column is false why is anything showing up in the admin? Curious. It is bed time in Europe, so I hope to check out your pull request in the AM. Cheers and thanks for looking at this.

haroldkyle commented 10 years ago

Well I worked on this a few hours but I don't have anything to pull quite yet. This issue was a bit more coding than I had anticipated.

If you look at /wp-admin/includes/class-wp-posts-list-table.php:754 you'll see that WP only checks for the show_ui property of the taxonomy. show_admin_column is not related to its appearance in Quick Edit. The same is true in the row values populated in /wp-admin/includes/template.php:324.

The problem here is that there is no hook to override the way WP Core structures the Quick Edit elements. So, I'm working to abandon using show_admin_column and WP filtering at all for this, and instead using JS to manipulate the DOM after rendering. This is going ok, but work remains. The values are not saving at the moment, for instance.

Which brings up the question: can you explain why it's important to send a plugin- and taxonomy-specific nonce with each Quick Edit save? I could conceivably get this working with JS but it seems strange especially because none of the other Quick Edit fields (outside the plugin) require their own nonce.

haroldkyle commented 10 years ago

I committed the changes that allow hierarchical taxonomies to work and to save. I did not send a pull request because of two outstanding issues with my code:

My changes are committed in my fork if you want to see. Here is the commit (with some code simply commented out for now). Off to a slew of meetings...

helgatheviking commented 10 years ago

If SourceTree doesn't flip out, I will try to get what I was working on today into a dev branch. It seems that we went in totally different directions to try to solve this. For some reason, I seem to remember not wanting to change the markup via jquery DOM manipulation, but I can no longer remember the why. Maybe it had to do with needing to generate the markup for the non-hierarchical radio buttons from scratch anyway?

If the column isn't being shown, and I don't disable the UI, I seem to get radio buttons for a hierarchical taxonomy, (and the value is already in WP's hidden div, so I've been trying to get it selected. I'm about to give up, because I am clearly selecting the right input, but it won't frakking check!!

http://i.imgur.com/jdASZE7.png

I think WP would probably even save that on it's own w/o my save routine. But then that still leaves me in a similar place to you in terms of having the non-hierarchical taxonomies.

I don't know if the nonces are all needed. They aren't blocking anything from saving as far as I can tell.

haroldkyle commented 10 years ago

Yeah, after spending some time on this I'm of the opinion that JS is the way to go to add the radio buttons:

The image that you shared looks like the problem that I initially reported. If it's like our problem. what happens next is that if you save the Quick Edit at its present state you remove all terms from the post. This caused some unexpected things to happen on our site earlier this week. Like, we suddenly only offered 3 ink colors for our printing instead of 80, whoops.

I have some time now so I'll try to work on the non-hierarchical markup.

Can you live without plugin- and taxonomy- specific nonces on the save? I would like to remove these.

haroldkyle commented 10 years ago

I do find myself wondering as I'm doing this why one would use radio buttons for non-hierarchical taxonomies. This really changes the interface and it seems counter to the intention of tags/non-hierarchical taxonomies. But I'm persisting for now...

helgatheviking commented 10 years ago

I'm inclined to agree with you on non-hierarchical taxonomies, but i probably can't remove it at this point. who knows if anyone is using it.

the markup can be filtered, that's what I have done with class.Walker_Category_Radio.php

have very little clue what heartbeat is or if this is really causing much server load. on the edit screen the markup is generated 1 time, and then cloned via JS every time the quick edit is clicked. that hardly seems like a resource drain.

haroldkyle commented 10 years ago

There isn't much server load, that's really not the issue...

OK I'll take another look at the Walker. That would be much better than the JS approach I've almost finished...so I'm reverting so I can better understand this. I will say that the markup can be (and is) filtered when show_admin_columns is true. This is how you swap the the textarea for a series of radio buttons. But when show_admin_columns is false WordPress does not trigger quick_edit_custom_box and...hence this issue. I wonder if there's another way to filter this on the backend. When I looked initially I was unable to find this...

haroldkyle commented 10 years ago

So I spent some time trying to find an opening, but I don't see a way to filter the output for non-hierarchical taxonomies that have don't have show_admin_columns. I think in this case JS is necessary to manipulate the Quick Edit elements.

Add to the list of advantages...

Not sure if you're going to approve of the direction I'm going but I'm happy to re-evaluate if you find a way to solve this situation!

helgatheviking commented 10 years ago

I'm only manipulating the show_ui property on the edit page. So far, I have yet to see any negative side effect.

But yeah, there is no entry-point into the quick edit... which was my problem when I built the thing in the first place. I commented on a trac ticket: https://core.trac.wordpress.org/ticket/26948 today as it's a pretty easy fix on the WP side, but I've not had a ton of luck getting things into core, so it could be years. As evidence, my original ticket on the same subject https://core.trac.wordpress.org/ticket/21840 is 20 months old.

I'm interested to see what you've come up with, but at the same time I have already devoted enough time to this today. I'm starting to lean towards just killing the UI on anything I can't manipulate and punt until the appropriate filter gets added. The work-around would have to be kind of complex.

haroldkyle commented 10 years ago

No problem. I feel your pain! Literally. Every time I work in the admin section of WP I get queasy.

I guess the only negative side effect of modifying show-ui is the difficulty debugging it. I was scratching my head for 10 minutes there. That was why I brought it up, I guess. I'm over it now.

I actually have this whole thing working now, I think. That said, I'd like to spend some time cleaning up code before I push another commit. The JS approach means we can trim down the PHP code, something that I haven't attempted to do yet. It bypass a lot of your hard work, I hope that you don't mind!!

helgatheviking commented 10 years ago

In general, what are you doing for the non-hierarchical taxonomies with no columns? I feel like I am pretty close with my approach too, except for that part.

On Wed, Apr 16, 2014 at 9:55 PM, Harold Kyle notifications@github.comwrote:

No problem. I feel your pain! Literally. Every time I work in the admin section of WP I get queasy.

I guess the only negative side effect of modifying show-ui is the difficulty debugging it. I was scratching my head for 10 minutes there. That was why I brought it up, I guess. I'm over it now.

I actually have this whole thing working now, I think. That said, I'd like to spend some time cleaning up code before I push another commit. The JS approach means we can trim down the PHP code, something that I haven't attempted to do yet. It bypass a lot of your hard work, I hope that you don't mind!!

— Reply to this email directly or view it on GitHubhttps://github.com/helgatheviking/Radio-Buttons-for-Taxonomies/issues/17#issuecomment-40644382 .

haroldkyle commented 10 years ago

Localizing the terms for JS to populate.

So, here's my commit. I didn't realize we were both working on this concurrently. Anyway, let me know if you'd like me to send a pull request. All appears to be working, as far as I can tell. Hopefully I didn't cut too much.

helgatheviking commented 10 years ago

Yeah, sorry I should've said. I have actually cut more... b/c if you leave the input names as tax_input[] instead of radio_tax_input[] you don't really need a save routine. Do send the pull request.. though I'd prefer to get a dev branch up for you to pull against. SourceTree, work damn you!

I'll definitely want to merge the localization/JS trickery.

What do you have against the nonce? I've always seen it in every meta box tutorial, so I just assumed it was a good security step. But if I drop the save routine, then it isn't needed.

haroldkyle commented 10 years ago

Cut away! I'd rather WP Core be handling as much as possible.

Nonces. Well, you really don't want to get me started on this topic. I'm not a huge fan of WordPress's implementation of nonces and especially in this situation they add needless complexity. WordPress is trusting all the other values sent in the POST request, and that suffices for me. But that's just my opinion--if you love nonces I'm sure I could pass them to the front end with script localization.

Speaking of security, one thing that did raise an eyebrow (which I didn't look into and could report separately) was the "Check permissions" in WordPress_Radio_Taxonomy::save_taxonomy_term(), which just checks post editing capabilities. Perhaps we should check capability of editing the terms for the given taxonomy?

helgatheviking commented 10 years ago

I'm pretty sure that WP isn't trusting everything. I think they have their own nonce.

The more I am thinking about it the more I think we'll need to keep the save routine. It is kind of edge case, but without it technically you can still have posts with more than 1 term (maybe a residual from before RB4T was implemented).

I am leaving on vacation in the morning, so I won't be able to merge (automatically or more likely, manually) until at least next week.

Thanks for the work on this and have a great weekend.

helgatheviking commented 10 years ago

@haroldkyle So I have finally come back to this neglected plugin. Would you mind testing my dev2 branch?

At the moment, I've decided to go in a different direction than you did in your PR. Not that I didn't appreciate your effort, but I think what I've done is more inline w/ how it was already working. I was already "hacking" the global $wp_taxonomies variable and realized that if on the edit.php screen I switched all the radio taxonomies to "hierarchical" that WP would show checklist-turned-radioes automatically without JS rendering.

That branch should (in theory) now work properly for quick edit and bulk edit. I've kept the nonces. I think they're important, they're a little simpler now though (imo) and in the words of my Grandma, they aren't eating anything. ;) Oh and I added a capability check to the save routine too.

If you find anything, open up a new ticket. Thanks again!

haroldkyle commented 10 years ago

I'm curious if these fixes got committed to the version in the WordPress SVN? Is it safe to use Quick Edit again with this plugin?

helgatheviking commented 10 years ago

Yes I believe so. The repo version is at 1.7 which is the same as the master branch here. And the changelog also confirms these changes were committed: