| Project: | Forum Access |
| Version: | 6.x-1.x-dev |
| Component: | Code |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
I have a 'free tagging' vocabulary that allows new terms to be added by users. This vocabulary is not used by any forums. After recently enabling forum access, most (not all) of the terms in this vocabulary have become hidden to normal users, which means that a lot of duplicate terms are showing up in the vocabulary as users re-add terms they can't see. It seems that the terms that are visible have been recently added pontentially after forum access was enabled.
Only the main admin user can see all terms in the vocabulary.
I enabled logging on mysql and found that, for normal users, it was executing this query for taxonomy terms when I using the free tagging typeahead feature:
SELECT DISTINCT(t.tid), t.name FROM term_data t
LEFT JOIN forum_access fa ON t.tid = fa.tid
LEFT JOIN acl acl_fa ON acl_fa.name = t.tid AND acl_fa.module = 'forum_access'
LEFT JOIN acl_user aclu_fa ON aclu_fa.acl_id = acl_fa.acl_id AND aclu_fa.uid = 5 WHERE ((fa.grant_view >= 1 AND fa.rid IN (2, 3, 4)) OR fa.tid IS NULL OR aclu_fa.uid = 5) AND ( t.vid = 2 AND LOWER(t.name) LIKE LOWER('%Syb%')) LIMIT 0, 10However, it was using this query for the main admin user:
SELECT t.tid, t.name FROM term_data t WHERE t.vid = 2 AND LOWER(t.name) LIKE LOWER('%Syb%') LIMIT 0, 10Is there some configuration setting that I am missing? Or does this look like a bug?
Comments
#1
FA is intentionally hiding the forum vocabulary terms for forums where the user doesn't have at least View access, and it's intentionally NOT doing this for user 1.
However, it should not hide terms of other vocabularies. This does indeed look like a bug. We need to figure out how to limit the filtering to the forum vocabulary...
This is on node/forum/add and node/edit, right?
I'm a bit surprised that no one has tried this before.
#2
I've been unable to reproduce the tagging vocabulary behavior that you mention.
In addition I've added a (non-tagging) vocabulary to the forum topic type, and preparing the drop-down selection list creates the following query, which is very similar to what you show:
SELECT DISTINCT(t.tid), t.*, parentFROM term_data t
INNER JOIN term_hierarchy h ON t.tid = h.tid
LEFT JOIN forum_access fa ON t.tid = fa.tid
LEFT JOIN acl acl_fa ON acl_fa.name = t.tid AND acl_fa.module = 'forum_access'
LEFT JOIN acl_user aclu_fa ON aclu_fa.acl_id = acl_fa.acl_id AND aclu_fa.uid = 4
WHERE
((fa.grant_view >= 1 AND fa.rid IN (2, 3, 4))
OR fa.tid IS NULL
OR aclu_fa.uid = 4)
AND ( t.vid = 2 )
ORDER BY weight, name
The essential part here is
fa.tid IS NULL— it ensures that tids that are not in the {forum_access} table are not filtered out. I think this works correctly.#3
I have temporarily disabled forum access in order to hit milestone this week. I will try to reproduce the issue next week and post my results.
#4
Confirming. The bug is not obvious, you need several users with different user roles/permissions to uncover it.
While working on the attached patch, which solves the immediate problem at hand, I realized that the real cause must be elsewhere. Forum access stores rows in {forum_access} for terms in ALL vocabularies, which effectively breaks the failover in the rewritten SQL query:
// Allow access to unrelated/unconfigured terms.// @todo fa.tid should be sufficient, but the module currently stores rows
// for terms in *all* vocabularies.
$sql['where'] .= "OR fa.tid IS NULL OR acl_fa.acl_id IS NULL ";
I've additionally cleaned up those insane table aliases. Took me quite some time to see the squeezed-in 'u' in one of the joined table aliases... not sure why such cryptic aliases were chosen.
#5
If you have non-Forum terms in {forum_access} then there's obviously a problem, that needs to be fixed, as you point out. Massaging forum_access_db_rewrite_sql() is not the solution. Please update to the -dev version. I updated the forum_access_db_rewrite_sql() to clarify it a bit; the SQL in forum_access_db_rewrite_sql() remains unchanged, but I made a number of other fixes that you want to have.
I think you may be bitten by #422218: $node->tid can get the wrong tid if a forum content type has additional vocabularies. Unfortunately, the prima donna in charge (I don't mean the D7 maintainer, this is D6) cannot be bothered to consider fixing it — maybe you can add some push to the issue. It's frustrating that we waste time over known core issues that hibernate for no good reason.
BTW, I was disappointed to see you comment on IRC that the open issues list on the Forum Access front page gave you a bad impression of FA. These are all core issues that I've tried to get fixed. I'm trying hard to be as straight-forward about all known problems; why are you holding this against FA?
You'll have to ask merlinofchaos. It's a matter of taste, which is somewhat subjective. I'm fine with them, whether insane or not — you didn't expect two tables to have the same alias, did you? ;-)
I'm hearing a bit of prima donna in your voice, too. This is sad, because it hurts Drupal.
#6
1) This patch is against latest code in CVS already.
2) #422218: $node->tid can get the wrong tid if a forum content type has additional vocabularies is very unlikely to be the cause here, because in my case, taxonomy terms are removed from entirely different node types using entirely different vocabularies, not forum nodes. RTBC'ed that patch anyway.
3) The project page can be updated, because #284392: db_rewrite_sql causing issues with DISTINCT has been committed. Thus, people could technically use 6.16-dev to resolve at least 1/3 of the known issues.
4) Regardless of who worked on the code of this module, the coding style needs work. Especially that hook_db_rewrite_sql() implementation.
5) -- WTF? Did you get up on the wrong side of the bed? ;)
To get back on focus, 2) holds the important detail, sorry for not mentioning it previously. Changed the issue title accordingly.
#7
I'm sorry, I don't have enough time right now.
Thanks for RTBCing!
If it's not that issue, then we need to find out where those non-forum tids came from. FA is certainly NOT supposed to store non-forum tids in {forum_access}. I'll have to dig into the code as soon as I have time.
#8
1) There's only one
INSERT INTO {forum_access} ...statement, in _forum_access_form_submit(). It setstidto$form_state['values']['tid']from admin/content/forum/edit/forum/TID or admin/content/forum/edit/container/TID. I won't accept a patch that saysbecause it's not true. Please find out what the real problem is on your site.
2) Yes, you're right, it must be a different issue. There's no way the $node->tid could get into {forum_access}.tid. But thank you for the push!
3) Thanks for pointing that out. I've added a note to the project page.
4) Correction: The "acl" vs. "aclu" thing (which is what you specifically complained about) was done by merlin. I added the "_fa" suffix for Forum Access to avoid conflicts with Image Gallery Access, which adds similar clauses using the "_iga" suffix. Removing the suffix for FA (as you've done) and consequently for IGA, too, would make it impossible to use both modules together, even though they typically have no overlap. The same holds for other ACL clients such as Content Access, of course.
5) Unfortunately, people with very high Drupal merits (such as yourself) are often quick to denigrate everything that is not exactly to their liking. Just because you prefer one style does not automatically mean that another style is "insane." If the big shots would show a tiny bit of modesty rather than trying hard to outdo each other in bashing code (and sometimes even people), this would be a nicer place.
I prefer "aclu" over "acl_user" because clunky database aliases make the SQL harder to read. I go for an alias with an underscore ("aclu_fa") only to mark the "_fa" as a suffix.
#9
1) Could it be that you have another module altering the forum/container administration form? And that other module would cause
$form_state['values']['tid']to be set to some other tid? I can't see how non-forum tids could get into the {forum_access} table with only Forum and Forum Access.Search for $form_id == 'forum_form_container' or 'forum_form_forum'...
#10
I've just installed FA into my 6.16 and have a similar problem. Nonpriveleged users see a empty list of tags for non-forum terms while adding new non-forum node.
If you say that forum_access table can't contain non-forum terms tnan something wrong with forum_access_install(). Because the following query does exactly what it should do.
db_query("INSERT INTO {forum_access} (tid, rid, grant_view, grant_update, grant_delete, grant_create, priority)
SELECT t.tid, %d, 1, 0, 0, %d, 0
FROM {forum_access} fa RIGHT JOIN {term_data} t ON fa.tid = t.tid AND t.vid = %d AND fa.rid <> %d
WHERE fa.tid IS NULL",
$rid, $grant_create, DRUPAL_AUTHENTICATED_RID, $vid
);
I moved AND t.vid = %d into WHERE clause. Now I hope it works fine.
db_query("INSERT INTO {forum_access} (tid, rid, grant_view, grant_update, grant_delete, grant_create, priority)
SELECT t.tid, %d, 1, 0, 0, %d, 0
FROM {forum_access} fa RIGHT JOIN {term_data} t ON fa.tid = t.tid AND fa.rid <> %d
WHERE fa.tid IS NULL AND t.vid = %d",
$rid, $grant_create, DRUPAL_AUTHENTICATED_RID, $vid
);
#11
Oh, that's http://drupalcode.org/viewvc/drupal/contributions/modules/forum_access/f..., introduced in #493044: Permissions not set properly. That's nasty indeed, thanks for tracking it down!
The proper syntax is
db_query("INSERT INTO {forum_access} (tid, rid, grant_view, grant_update, grant_delete, grant_create, priority)
SELECT t.tid, %d, 1, 0, 0, %d, 0
FROM {forum_access} fa RIGHT JOIN {term_data} t ON fa.tid = t.tid
WHERE fa.tid IS NULL AND t.vid = %d AND fa.rid <> %d",
$rid, $grant_create, DRUPAL_AUTHENTICATED_RID, $vid
);
Now we need a hook_update_NNN() function to remove those bogus rows...
#12
Will this
select * from forum_access where tid not in (select tid from term_data where vid = %d)get them (with %d = forum vid)?
#13
I doubt if arguments "DRUPAL_AUTHENTICATED_RID, $vid" in your query are in proper order
WHERE fa.tid IS NULL AND t.vid = %d AND fa.rid <> %d",$rid, $grant_create, DRUPAL_AUTHENTICATED_RID, $vid
I installed FA using my version of query so now there is no any non-forum tids in forum_access
#14
Right!
db_query("INSERT INTO {forum_access} (tid, rid, grant_view, grant_update, grant_delete, grant_create, priority)
SELECT t.tid, %d, 1, 0, 0, %d, 0
FROM {forum_access} fa RIGHT JOIN {term_data} t ON fa.tid = t.tid
WHERE fa.tid IS NULL AND t.vid = %d AND fa.rid <> %d",
$rid, $grant_create, $vid, DRUPAL_AUTHENTICATED_RID
);
Whew...
According to
http://dev.mysql.com/doc/refman/5.0/en/join.html
only
fa.tid = t.tidshould be in the ON clause.#15
I've fixed the initialization of the {forum_access} table in forum_access_install() and added a hook_update_NNN() to remove any rows from the table that don't belong there.
http://drupal.org/cvs?commit=340070
Update to the -dev version (will be repackaged within 12h) and run update.php. It will report the number of bogus rows it has purged. Rebuilding permissions is not necessary. Please let us know how it goes!
Thanks for tracking this down, kwinto!
#16
I was having a similar problem with regular taxonomy terms - they didn't use free tagging - but only showed up for the admin - not authenticated users.
I am on a deadline and couldn't take the time to use the entire patch. But, I did use the delete construct and it, in fact, removed all the non-forum-related taxonomy terms from the forum_access table.
And, that solved the problem.
Thanks much!
#17
Ok, that's encouraging. Thanks for taking the time to tell us, keving_65!
The other part of the patch is only used when FA is newly installed on a site that already has forums. Its purpose is to pre-populate the {forum_access} table with the default permissions (read access for anonymous, and read/write access for authenticated).
Any volunteers to verify this? You'd have to not only disable but uninstall FA and reinstall, and then check the {forum_access} table.
#18
Automatically closed -- issue fixed for 2 weeks with no activity.