Taxonomy module still has essentially pre-FAPI code using raw $_POST, and hasn't been fully updated for the new menu system.

CommentFileSizeAuthor
#12 taxonomy-kill-post-163297-12.patch2.51 KBpwolanin
#9 taxonomy-6.x_vocab_fapi_9.patch3.33 KBHorsePunchKid
#9 taxonomy-6.x_vocab_term_fapi_9.patch4.41 KBHorsePunchKid
#8 taxonomy-kill-post-163297-8.patch2.42 KBpwolanin
#5 taxonomy-6.x_vocab_fapi_5.patch3.22 KBHorsePunchKid
#4 taxonomy-6.x_vocab_fapi_4.patch3.53 KBHorsePunchKid

Comments

catch’s picture

catch
he/him
Primary language English
commented

These are the functions using $_GET or $_POST from taxonomy.admin.inc:

function taxonomy_admin_vocabulary_edit($vocabulary)
function taxonomy_admin_term_edit($tid)
taxonomy_overview_terms

catch’s picture

catch
he/him
Primary language English
commented

taxonomy_overview_terms is only using $_GET for paging, I assume that's fine.

However sure it shouldn't have comments like this in it:

    $GLOBALS['pager_page_array'][] = $start_from;  // FIXME
    $GLOBALS['pager_total'][] = intval($total_entries / $page_increment) + 1; // FIXME

// FIXME

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Still not fixed.

HorsePunchKid’s picture

HorsePunchKid commented
Status: Active » Needs review
StatusFileSize
new taxonomy-6.x_vocab_fapi_4.patch3.53 KB

Something like this? I just modeled it very closely on how the term code worked.

HorsePunchKid’s picture

HorsePunchKid commented
StatusFileSize
new taxonomy-6.x_vocab_fapi_5.patch3.22 KB

Simpler patch with the same result; turns out taxonomy_admin_vocabulary_edit was entirely unnecessary.

pwolanin’s picture

pwolanin commented
Status: Needs review » Needs work

Hmm, I'm not sure what happens when you return a form from a _submit function - I don't think that's quite right.

Should probably be done more in the style of my fix for user module (even though that has a bit of tab ugliness going on)

pwolanin’s picture

pwolanin commented
Status: Needs work » Needs review

oh, wait - I mis-read the patch code. Actually looks pretty good.

however, since removing function taxonomy_admin_vocabulary_edit($vocabulary) could be considered an API change, probably better to just leave it as a wrapper.

pwolanin’s picture

pwolanin commented
StatusFileSize
new taxonomy-kill-post-163297-8.patch2.42 KB

Also, the code is a little fragile, since it relies on buttons on two different forms having the same name.

Here's a slightly modified version.

HorsePunchKid’s picture

HorsePunchKid commented
StatusFileSize
new taxonomy-6.x_vocab_term_fapi_9.patch4.41 KB
new taxonomy-6.x_vocab_fapi_9.patch3.33 KB

I like that change to t_v_confirm_delete (which could be applied to the term deletion form, too), but why keep taxonomy_admin_vocabulary_edit if all it does is call drupal_get_form? I guess I felt removing that was in the spirit of "cleaning up menu usage".

Hybrid patch attached; also a patch with the analogous change for term deletion. Both seem to work fine.

HorsePunchKid’s picture

HorsePunchKid commented

If you prefer #8, note that the // Rebuild comment in taxonomy_form_vocabulary_submit needs s/term/volcabulary. (That was a problem in my earlier patches, too.)

pwolanin’s picture

pwolanin commented

@HorsePunchKid - I'd happily support removing that function in HEAD, but for 6.x, I think removing a public function counts as an API change - for example, there may be contrib modules already ported that use it (doubtful, but possible).

pwolanin’s picture

pwolanin commented
Title: remove $_POST and clean up menu usage in taxonomy module » remove $_POST in taxonomy module
StatusFileSize
new taxonomy-kill-post-163297-12.patch2.51 KB

corrected code comment and swapped $edit['vid'] for $form_state['values']['vid']

pwolanin’s picture

pwolanin commented

@HorsePunchKid - also, fixing the id of the term deletion form might be a good idea, but open a separate issue for that.

bennybobw’s picture

bennybobw commented
Status: Needs review » Reviewed & tested by the community

Tested on HEAD. Applies cleanly, code looks good. RTBC.

HorsePunchKid’s picture

HorsePunchKid commented

Thanks for the helpful info, pwolanin. Patch looks good!

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Version: 6.x-dev » 7.x-dev

Well, book, comment, poll, search, system, upload, user modules all use _POST instead of FAPI constructs, and there is no bug here as far as I understand, so let's our continue migration to cleaner FAPI code in Drupal 7.

pwolanin’s picture

pwolanin commented

@Gabor - book module uses it in the context of the ahah method, not for a form per se.

Using POST for a deletion form does pose some risk of CSFR requests, though perhaps Heine can weigh in on the seriousness of it.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

Anonymous’s picture

Anonymous (not verified) commented
Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.