entity_translation_node_tab_access() overrides all other access checks for translatable nodes. Wouldn't it be better if it worked in addition to them, instead of in place of them?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

douggreen’s picture

Status: Active » Needs review
FileSize
1 KB

Patch attached

Status: Needs review » Needs work

The last submitted patch, 1978616.patch, failed testing.

plach’s picture

I am not sure I understand the goal of this patch: the current code checks whether core node translation or entity translation is enabled for the given node. In the first case it proxies access-control to the implementing module otherwise it performs ET-specific access control. I wouldn't expect other access callbacks to be in place in the latter case. What am I missing?

Anyway, the current patch does not look right to me, as it's mixing unrelated access callbacks.

douggreen’s picture

The problem is that it isn't proxied when entity_translation_node('node', $node) is FALSE. translation_supported_type is not TRUE for my node because I'm only using entity_translation and this is set to ENTITY_TRANSLATION_ENABLED and not TRANSLATION_ENABLED.

plach’s picture

Let me recap: the node/%/translate route is shared between (at least) ET and the core Translation module. When node translation is enabled access calbacks are proxied because we don't want to interfere with core Translation (or whatever module, such as i18n_node, overrides its route). Instead when entity translation is enabled for the content type the only node access callback that is supposed to be executed is the one defined in the entity translation info, that is entity_translation_node_tab_access() for nodes. What I don't understand is what access callbacks are being skipped that you think should be called.

douggreen’s picture

"Instead when entity translation is enabled for the content type the only node access callback that is supposed to be executed is the one defined in the entity translation info, that is entity_translation_node_tab_access() for nodes."

This prevents any custom or contrib module from having a say in the access callback. You are assuming that the only modules that care about this might be the two or three translation modules you mention.

I suspect there is a use-case you are defending that I am not seeing. But there is also a use-case here that I am either doing wrong, or you are not seeing.

In my case, each node can only be translated into certain languages. And I also modify the /translate tab using hook_preprocess_table() to only show the languages that the node supports. So I want to hide the /translate tab when the node has no available languages for translation. To do this, I've implemented hook_implements_alter() and hook_menu_alter(0 and proxied the access arguments through my own access check. But because entity translation module believes that it has the only say on this access, my module access checks never get called.

plach’s picture

Going the hook_menu_alter way here has the drawback of forcing us to skip the check for the translation method enabled for the current node. This may result in combining access checks that are meant to be used with different translation methods.

I think the proper way to take control of the access checks for the /translate route, but only when the node is entity translation-enabled, is altering the node entity info and specifying your custom access callback there. You can read the details here:

http://drupalcode.org/project/entity_translation.git/blob/refs/heads/7.x...

I'd suggest to call entity_translation_node_tab_access() from your access callback to ensure all the needed checks are performed.

douggreen’s picture

I tried changing the entity as you suggested, using hook_entity_info_alter() and discovered that my callback was never called (on node/% pages, for example). So I reviewed the code again. And I think the problem is with the /translate tab, which is menu altered by entity_translation_node_menu_alter(). Is the problem that entity_translation_node_tab_access is used by both the entity and the menu callback, and that we want slightly different behavior for each???

plach’s picture

Status: Needs work » Needs review
FileSize
3.43 KB

Ok, now I see the issue, finally :)

I think this might fix it. We should probably do the same for page callback/arguments, but I have no time right now.

Status: Needs review » Needs work

The last submitted patch, et-tab_access-1978616-9.patch, failed testing.