The create and list operations need to be re-implemented in D7. At a minimum, they need to be ported to DBTNG and moved to hook_query_alter(). However, many issues have been caused by "guessing" whether the create or list op should be active based on arg() (see #903522: Move control of create into hook_form_alter() and hook_nodeapi() to prevent numerous problems). So, it might be better to use some of the new features of the D7 API instead.

E.g., see: hook_field_read_instance()

Comments

xjm’s picture

One plus of using hook_query_alter() is that other modules might go directly to the database instead of using the field API, and so altering queries is more of a sure thing.

The most important consideration is probably Views compatibility. I checked with dereine on IRC, and Views 7.x-3.x uses field_view_field() to load field data, which appears to eventually invokes hook_field_read_instance().

If we have any part of the create op in hook_form_alter(), it definitely needs to be factored to provide an API that can be used at other points in the node edit process (both for TAC internally and to make supporting other modules easier).

xjm’s picture

Issue tags: +d7 release

Adding tag.

xjm’s picture

The correct field hook is hook_field_attach_view_alter(). However, this hook will only allow us to remove the listing of the term from a node rendering (e.g., on a node page or in views). It does not restrict access to taxonomy/term/x, nor prevent nodes tagged with that term from being listed on that page, nor would it prevent the term from being listed in (e.g.) a taxonomy view. So, to avoid using hook_query_alter(), we'd need to handle these cases as well.

Other possibilities include the taxonomy hooks hook_taxonomy_term_load() and hook_taxonomy_term_view_alter(). However, it appears these do not provide sufficient support for views. Neither can alter the output of Views' All taxonomy terms field type, for example. For that matter, nor can hook_field_attach_view_alter(). Furthermore, the only tag on the query for All taxonomy terms appears to be term_access, so there is no obvious way to distinguish it from other situations.

At this point, it looks like it may be a matter of letting list take away terms everywhere with hook_query_term_access_alter(), and then adding the terms back somehow in the taxonomy field editing widget. Other scenarios in which list should not apply would need to be documented and handled on a case-by-case basis.

xjm’s picture

Possible solution suggested by catch on IRC: Override all taxonomy field widgets somehow so that they do not use the tagged query (or so that they add their own tag to the query). See: hook_field_info_alter() and hook_field_widget_info_alter().

xjm’s picture

Unfortunately, the query tags are added at the lowest level in the taxonomy API. After some investigation, it became clear that we'd have to duplicate half of taxonomy.module for the solution in #4. It could still be used as a workaround: If the user is editing the field somewhere other than the node add/edit forms, we could provide a "create-safe" widget for those circumstances.

For the moment, however, I'm leaning toward this solution:

  1. API for which op is active, if either: something like taxonomy_access_create_active() and taxonomy_access_list_active(). (Could even make these alterable if there is a demand for it.)
  2. List is only active when create is not active, and the user does not have administer taxonomy privileges. (Add administer TAC privilege? Grant automatically to roles with whatever permission we used before?). This activates our hook_query_term_access_alter().
  3. Create is only active when menu_get_item()'s callback is either node_add or node_page_edit.
  4. Not sure what to do for places where list should be active on node add/edit paths (e.g., menus, blocks, etc. that contain taxonomy).

Not a perfect solution, but still a step up from D6.

xjm’s picture

I found a better solution. We create a static flag that disables the list op's hook_query_alter() only while values are being generated for a taxonomy field (by overriding the options list), and again during node validation and saving. It works!

The only case this doesn't cover is taxonomy widgets not provided by core taxonomy. Perhaps the flag should be set for the duration of a node form generation.

xjm’s picture

Status: Active » Needs work
xjm’s picture

http://drupalcode.org/project/taxonomy_access.git/commit/f78ea12 disables the list op during field editing. Edit: It appears to have introduced a regression when user does not have any list permissions. (We need to use IS NULL or something in the query rather than IN ()).

Outstanding questions:

  1. What about term reference autocomplete and new term creation?
  2. What about validation of term reference fields on non-node entities?
  3. What about (hypothetical) non-term reference taxonomy fields that other modules may create?
  4. Do taxonomy/term pages behave as expected?
  5. Do fields hide themselves properly when no terms are listed?

The create grant itself is not yet implemented.

xjm’s picture

Just confirmed that list permissions are still (incorrectly) filtering autocomplete fields, both the initial listing and the AJAX lookup. Users can add new terms; however, they cannot see these terms on node edit.

xjm’s picture

Issue tags: +Needs documentation

After many gruesome battles and much gnashing of teeth, the autocomplete widget is working:
http://drupalcode.org/project/taxonomy_access.git/commit/72645f4

We should possibly move the list disable flagging into callbacks added by hook_form_alter() and out of hook_node_validate()/hook_node_update() for better portability across entity types. There's still the issue of non-core widgets and fields, but that might be better addressed when we have a real example.

xjm’s picture

It appears that the current mechanism for create is bugged with hierarchical select's widget. (Multiselect's works fine.) Edit: HS itself is quite buggy at present.

Status: Fixed » Closed (fixed)
Issue tags: -Needs documentation, -d7 release

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