The current implementation tries to pass the old taxonomy terms in comment_alter_taxonomy_original_taxonomy in order to preserve non alterable vocabularies during taxonomy_node_save.

This refactoring output all taxonomy widgets and simply disable (ie. transform the widgets to 'value') those which are not alterable by the current user.

This way, both _comment_alter_taxonomy_taxonomy_form_parse,comment_alter_taxonomy_validate and _comment_alter_taxonomy_all_taxonomy can go.

Also, _comment_alter_taxonomy_save_tids can directly get data from the database, because comment_alter_taxonomy has an heavier weight than taxonomy.

Refactored several other code (using isset() instead of the slow array_key_exists() in several places).

Also fixes #223073: Comments by users without proper permissions can change terms.

CommentFileSizeAuthor
#10 comment-alter-taxonomy-refactor-8.patch27.99 KBaclight
#9 comment-alter-taxonomy-refactor-7.patch26.95 KBaclight
#7 comment-alter-taxonomy-refactor-6.patch26.95 KBaclight
#6 comment-alter-taxonomy-refactor-5.patch25.23 KBaclight
#5 comment-alter-taxonomy-refactor-5.patch25.18 KBdamien tournoud
#4 comment-alter-taxonomy-refactor-4.patch25.18 KBdamien tournoud
#3 comment-alter-taxonomy-refactor-3.patch24.96 KBdamien tournoud
#2 comment-alter-taxonomy-refactor-2.patch24.79 KBdamien tournoud
#1 comment-alter-taxonomy-refactor.patch24.78 KBdamien tournoud

Comments

damien tournoud’s picture

damien tournoud commented
Status: Active » Needs review
StatusFileSize
new comment-alter-taxonomy-refactor.patch24.78 KB
damien tournoud’s picture

damien tournoud commented
StatusFileSize
new comment-alter-taxonomy-refactor-2.patch24.79 KB

A user that don't have access to alter taxonomies could get a fieldset in some cases.

damien tournoud’s picture

damien tournoud commented
StatusFileSize
new comment-alter-taxonomy-refactor-3.patch24.96 KB

This should solve some strange behavior with users' permission.

damien tournoud’s picture

damien tournoud commented
StatusFileSize
new comment-alter-taxonomy-refactor-4.patch25.18 KB

Also add a fake term to prevent _comment_alter_taxonomy_save_tids to be called as long as the node has no term.

damien tournoud’s picture

damien tournoud commented
StatusFileSize
new comment-alter-taxonomy-refactor-5.patch25.18 KB

Silly parse error.

aclight’s picture

aclight commented
StatusFileSize
new comment-alter-taxonomy-refactor-5.patch25.23 KB

Reroll to fix a typo in line 300

aclight’s picture

aclight commented
StatusFileSize
new comment-alter-taxonomy-refactor-6.patch26.95 KB

Here's some cleanup and increased documentation.

There's still one problem with this patch. I haven't worked out the exact problem, but basically what I did was I had several vocabs that were for issues, but I had the cat module disabled. I created an issue and assigned some terms to it.

I then enabled the module, but didn't have any of the vocabularies enabled for cat. I then followed up to the issue. Then I enabled cat for one of the vocabularies for issues, then created a comment and changed the value of one of the terms.

I got some db errors:

    * user warning: Duplicate entry '207-0-0' for key 1 query: _comment_alter_taxonomy_save_tids INSERT INTO comment_alter_taxonomy (nid, cid, tid) VALUES (207, 0, 0) in /Applications/MAMP/htdocs/freetags/drupal/includes/database.mysql.inc on line 172.
    * user warning: Duplicate entry '207-0-48' for key 1 query: _comment_alter_taxonomy_save_tids INSERT INTO comment_alter_taxonomy (nid, cid, tid) VALUES(207, 0, 48) in /Applications/MAMP/htdocs/freetags/drupal/includes/database.mysql.inc on line 172.
    * user warning: Duplicate entry '207-0-152' for key 1 query: _comment_alter_taxonomy_save_tids INSERT INTO comment_alter_taxonomy (nid, cid, tid) VALUES(207, 0, 152) in /Applications/MAMP/htdocs/freetags/drupal/includes/database.mysql.inc on line 172.
    * user warning: Duplicate entry '207-0-55' for key 1 query: _comment_alter_taxonomy_save_tids INSERT INTO comment_alter_taxonomy (nid, cid, tid) VALUES(207, 0, 55) in /Applications/MAMP/htdocs/freetags/drupal/includes/database.mysql.inc on line 172.
    * user warning: Duplicate entry '207-0-76' for key 1 query: _comment_alter_taxonomy_save_tids INSERT INTO comment_alter_taxonomy (nid, cid, tid) VALUES(207, 0, 76) in /Applications/MAMP/htdocs/freetags/drupal/includes/database.mysql.inc on line 172.
    * user warning: Duplicate entry '207-0-75' for key 1 query: _comment_alter_taxonomy_save_tids INSERT INTO comment_alter_taxonomy (nid, cid, tid) VALUES(207, 0, 75) in /Applications/MAMP/htdocs/freetags/drupal/includes/database.mysql.inc on line 172.

I have to head out now and will try to finish reviewing the patch tonight, if I have time.

Thanks for working on this so much, it will be a great improvement.

damien tournoud’s picture

damien tournoud commented 
+      if (!db_result(db_query_range('SELECT nid FROM {comment_alter_taxonomy} WHERE nid = %d AND cid = 0', $arg['nid']))) {

You removed the 0, 1 at the end of that line, which brakes the thing and leads to the behavior you described.

aclight’s picture

aclight commented
StatusFileSize
new comment-alter-taxonomy-refactor-7.patch26.95 KB

Yep, you're right. Attached patch fixes this.

Another problem I noticed.

Disable cat. Create a new issue, and assign some terms to it.
Create 2 or 3 comments on this issue.
Enable cat, but don't enable cat for any vocabularies.
Create another follow up on the issue.
Enable cat for all the vocabularies you used in the first step.
When you view the issue, the terms appear to have been removed in the first comment, and then added back later on, even though in reality you only assigned the terms on the original issue node.

aclight’s picture

aclight commented
StatusFileSize
new comment-alter-taxonomy-refactor-8.patch27.99 KB

Here's the most recent patch with a few more corrections and comment changes.

aclight’s picture

aclight commented
Status: Needs review » Fixed

Fixed with http://drupal.org/cvs?commit=143033 (patch in comment #10).

I've opened a separate issue for the bug I describe in comment #9 at #314260: Strange behavior when comment_alter_taxonomy module is enabled after node is already created since that'll make it easier to review the fix for that bug.

Thanks a lot for writing this patch. It simplifies the form quite a lot.

Anonymous’s picture

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

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