node save (on edit) removes tags

subscribe

This is a known bug.

Until it is solved:
'Update/Delete' permissions supposed to be Administrators' permissions (eg. for content administrators), so that you should give these administrators 'Create' permission as well.

Any indications on the expected time for a fix? And will it require any patch(s) to Core to function correctly?

A commit on this issue, http://drupal.org/node/93086, would fix this problem. However, a workaround solution can be provided for TAC if required (due to a non-commit of the other issue)

It seems a "core fix" may not happen, so in the event that it doesn't, here's a patch workaround to overcome this "bad behaviour" of core (see the other issue thread).

Actually, you can bin that patch. I had the module weight set to -9 as an experiment and TAC installs itself as +9 (which means the order in which things happen I got wrong, so adding the term_data back in to $node->taxonomy during _nodeapi() doesn't work).

I'll rework this patch shortly so that the data gets put back in at the correct place.

and here it is, reworked for module weight of +9 (only needed to add one line)

Good job! Thanks for the code. :)

I have to test it.
I thought about a more 'drupalish' way to do it. What do you think about putting 'tac_protected' terms into the form as a hidden field, instead of global variable tac_protected? Then saving terms into database with [form]_submit function instead of nodeapi? I am not sure, if this way, we can put this terms into $node->taxonomy. This is just an idea, i have not tried.

I would also put a 'DELETE FROM {term_node} ...' query to make sure and avoid duplicate entry errors.

Thanks for your help again.

I thought about a more 'drupalish' way to do it. What do you think about putting 'tac_protected' terms into the form as a hidden field, instead of global variable tac_protected? Then saving terms into database with [form]_submit function instead of nodeapi?

Yup, this would also work. Basically, anything that can "remember" the lost data after Drupal has "saved" it will work. The work I did was more a proof of concept, I just used $_SESSION to store the data. I had quickly discussed your method with Budda a few days ago but for a "quick try/roll out" I went with $_SESSION. No harm in doing a "neater" Drupal based solution though ;)

I am not sure, if this way, we can put this terms into $node->taxonomy. This is just an idea, i have not tried.

You can't use this mechansm as TAC installs with a weight of +9 so the standard Drupal node_save() of $node->taxonomy has passed you by before you get the opertunity to re-insert the missing data back into the array (you'll see that the patch does re-insert the data in nodeapi but to get it into the database requires direct database manipulation). Actually, a neater solution is taxonomy_node_save() to re-insert the array back ;)

I would also put a 'DELETE FROM {term_node} ...' query to make sure and avoid duplicate entry errors.

Yes, good point actually, I found I had a few of these until I fine tuned the code to only insert the correct data. But doing a DELETE before the INSERT would avoid any "data creep" that could cause a warning like this.

regards,
--AjK

Keve,

Here's a 'drupalish' version I believe you'd be far happier to commit.

Please note, this patch was sponsored by Ixis

regards,
--AjK

I should have added a note, the patch in #10 relies totally on the modules install weight being positive (it installs as +9 as you know). Should the module weight = 0 (or lower) then this patch will not work and the only way to proceed with a bullet proof version would be to revert to using the $_SESSION variable (as used in the proof of concept patch in #7) and sticking the "restore" function in hook_exit().

But I presume we're safe with the current set-up and relying on the +9 module weighting.

regards,
--AjK

Good job. Thanks a lot (for you and to Ixis).
Do not worry, module weight will not change, other functions also depend on it.

I looked through the code, it seems good. I will test and commit it soon.
I will change some terminology: let the $tid be $protected_tids. (But you do not need to change patch, i will also make it for cvs.)

Did you test if node_access table is saved well (according to new terms)?

Did you test if node_access table is saved well (according to new terms)?

Nope, I only tested for 4.7. Not sure how this patch is involved with the node_access table? Doesn't appear to be to me.

regards,
--AjK

Sorry, I didn't mean "new terms" as 4.7 or 5.x.
I mean "new terms" as "protected terms". I was not clear.

I meant: e.g that if you have a term w/ DENY 'view' permission among $protected_terms than the node will be DENIED, for sure.

I meant: e.g that if you have a term w/ DENY 'view' permission among $protected_terms than the node will be DENIED, for sure.

Basically, the patch just returns to the term_node table items that were deleted that the current user is not supposed to have access to. This is because, internally, Drupal displays the category select form element via the taxonomy_form() function which has it's contents limited by db_rewrite_sql() (and TAC rewrites it to suppress disallowed terms). However, on update, Drupal deletes all terms for that node (regardless of access rights from TAC) and then adds terms back again (missing those the user didn't have access to).

This patch remedies that problem by storing those terms to which the user didn't have access and then restoring them after Drupal has "done it's bit". So, as to how the module worked previously prior to the patch, it should still continue to work after applying the patch.

I just tried / tested what you asked about a node being denied and it does indeed work. Basically, I had "userB" remove the taxonomies to which they had access. Once saved, userB can no long see or edit the node as they have effectively removed their own rights to it by removing the terms. So I believe the answer you are looking for is "yes" ;)

regards,
--AjK

Dear friends,

I had the same problem - TAC removed some term from a document when edited by people without admin. permission. I tried apply the patch 92355 - but the term are still removed...

Sorry for that this lasted so long.

AjK latest patch only worked with module weight set to -9 (lower than zero).
But with module weight 9, we cannot use form_validate to insert preserved terms into database. Node_api 'insert/update' should be used.

I commited to 4.7 with this latest patch. Please test it.

Minor fix to patch above: I needed to fix nodeapi: it should be in $op 'update', not in 'insert'.

I commited fix to HEAD, too.

AjK: thanks for all your help in this. I would be glad if you test this.

Keve, tested, looks good, test worked fine. Thanks for committing ;)