Consider alterable webform_menu_to_arg() implementation [#2097277]

Over in the webform_localization queue, there's an issue #1481594: Redirect webform config tabs to the original webform when using "single webform across translation set", dealing with the fact that when you use the webform_localization option to share a single webform across node translations, each node in the translation set still presents local tasks for their own specific webform (which in all cases but one is not the shared webform), and it would be far less confusing if those tasks all linked to the one which is shared.

My solution to this problem was to write a webform_menu_to_arg() function (which webform itself doesn't define), so that rather than trying to redirect all of the invalid (unused) URLs to the valid ones, instead all the webform URLs simply point to the correct webform in the first place, because the %webform_menu menu router placeholder looks up the desired webform nid, rather than using the current nid.

The code looks like this (it's calling another new function in the same custom patch, but you get the idea):

/**
 * Menu to_arg callback. If a single webform is used across all translations,
 * use the appropriate node ID for webform paths.
 */
function webform_menu_to_arg($arg, $map, $index) {
  if ($node = node_load($arg)) {
    if ($nid = webform_localization_single_webform_nid($node)) {
      $node = node_load($nid);
    }
  }
  return $node ? $node->nid : $arg;
}

The part I don't like is defining that function myself, when it's webform that defines the %webform_menu argument.

Would you consider implementing a stub definition for this in webform which invokes a hook/drupal_alter, so that modules like webform_localization could then simply implement the hook instead of stepping on webform's toes by defining the function directly?

I don't know if there are other use-cases for this, and I guess it's a small overhead, but it would make this approach to the localization problem seem cleaner.

Comment	File	Size	Author
#10	webform-webform_menu_to_arg-2097277-9.patch	1.5 KB	DanChadwick
#10

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #0.0

jweowu CreditAttribution: jweowu commented 25 September 2013 at 04:05

Issue summary:

View changes

Updated issue summary.

Comment #1

jweowu CreditAttribution: jweowu commented 24 March 2014 at 00:26

Status:

Active

» Needs review

I can't actually think of another use-case for this, so it might well be reasonable to simply let webform_localization define webform_menu_to_arg() on the assumption that it's the only module which is ever likely to need it, and simply re-visit the issue if a conflict ever does arise.

It would be nice to get some input from a webform maintainer first, though, so that if I submit a patch to webform_localization I can point back to this issue as justification for the approach.

Comment #2

jweowu CreditAttribution: jweowu commented 24 March 2014 at 00:31

Issue summary:

View changes

Comment #3

quicksketch

he/him

CreditAttribution: quicksketch commented 25 March 2014 at 22:05

Hi @jweowu! Would this patch still be necessary if we replaced %webform_menu with %node in hook_menu(), as suggested by #539164: menu_get_object() returns NULL at node/NID/submissions? I admit I'm not very familiar with the menu system's magical to_arg() functions.

Comment #4

jweowu CreditAttribution: jweowu commented 26 March 2014 at 03:27

Unfortunately that change would make this proposal unworkable :/

The _to_arg mechanism is definitely a little mysterious on first glance, but what it does is enable you to manipulate argument values when URLs are generated for a given menu router item (provided they are passed through _menu_translate() or _menu_link_translate(), which happens automatically for the likes of local tasks).

So for this example, when the menu system generates the local tasks for each of the node/%webform_menu/... paths, the default value for the %webform_menu argument (along with other data about the arguments) is passed to our webform_menu_to_arg() function, which returns the value to actually use in the URL being created.

So even though we might be generating the local tasks for node/123, the webform tasks could link to node/456/webform, node/456/submissions, etc... Extremely convenient in the case of this webform localization problem, when we want the webform local tasks for ALL nodes in the translation set to point at the same single webform.

(See also Dynamic argument replacement (wildcard) which shows some other use-cases for the _to_arg mechanism.)

Keeping a custom %webform_menu placeholder name means we have control over both the webform_menu_load() and webform_menu_to_arg() functionality. If we changed to %node then we would lose that control.

I guess you'd need to weigh the worth of that against the problems being addressed by the other issue.

Comment #5

DanChadwick CreditAttribution: DanChadwick commented 15 November 2014 at 02:50

Is this still a current request? I can see the wisdom in NOT defining a webform hook (well, menu loader for a wildcard defined in webform -- close enough) outside of webform.

OTOH, a drupal_alter seems heavy.

How about if webform defines the _to_arg() function and, if webform_localization defines it too, webform defers to it. Something like:

return function_exists('webform_localization_webform_menu_to_arg') ? webform_localization_webform_menu_to_arg(...) : $arg;

Patch welcome.

Comment #6

jweowu CreditAttribution: jweowu commented 15 November 2014 at 22:36

The _to_arg function can't be defined by multiple modules, because the function name is fixed.

For placeholder %webform_menu the function name must be webform_menu_to_arg()

The normal expectation is that _load and _to_arg functions would both be implemented by the module which introduced the placeholder in the first place.

What we have here is something of an unusual case. I think the options are still:

(a) let webform_localization define this function
(b) define it in webform, and call drupal_alter()

The cleaner option is (b), but in practice I don't think it would matter too much, unless someone comes up with another use-case for the alteration. If (a) was used, I think it would still be good to commit a comment in webform to document (near to or in webform_menu_load()) that the webform_menu_to_arg() implementation is deferred to the webform_localization module, so that if someone searches for that function name in this module, they find the important information.

Comment #7

DanChadwick CreditAttribution: DanChadwick commented 15 November 2014 at 22:41

@jweowu -- Your comment #6 confuses me, because I understood all that when I wrote #5, but you saying it again implies that somehow you thought I didn't get what you were saying.

I am proposing an option c) in which webform defines webform_menu_to_arg(), but defers to your function explicitly, rather than via drupal_alter. This seems lighter and faster, and doesn't create an API that no one but you would use. If you are amenable to this, we just need to agree on a name (I proposed webform_localization_webform_menu_to_arg()) and that I'll pass the arguments just as I got them. The body of my tiny function would be as in #5.

This presumes that you have not already defined webform_menu_to_arg() in your module. Let me know if you agree, and I'll get this right in. 7.x.4.2 is right around the corner.

Comment #8

jweowu CreditAttribution: jweowu commented 16 November 2014 at 11:14

Whoops; I entirely misinterpreted what you were suggesting. My apologies.

I think drupal_alter() is pretty efficient, so while I agree that it seems likely that nothing else would use the API, I'm also not sure we win very much by avoiding it (and doing so will make things a smidgen awkward if it was ever decided that the alter hook was desirable after all).

I don't particularly mind either way, though.

If we go with your proposal in #5, I think maybe we want a leading underscore on the function name, just so that it's clearer that it's not directly a _to_arg function by itself? (i.e. for some %webform_localization_webform_menu placeholder).

That would make it _webform_localization_webform_menu_to_arg()

Comment #9

16 November 2014 at 16:44

DanChadwick committed 541b6d3 on 7.x-4.x

Issue #2097277 by jweowu, DanChadwick: Added Consider deferred...

DanChadwick committed 2c4c96b on 8.x-4.x

Issue #2097277 by jweowu, DanChadwick: Added Consider deferred...

Comment #10

DanChadwick CreditAttribution: DanChadwick commented 16 November 2014 at 16:45

Version:	7.x-4.x-dev	» 8.x-4.x-dev
Status:	Needs review	» Fixed

File	Size
webform-webform_menu_to_arg-2097277-9.patch	1.5 KB

I hear you, but I think I'd like to stay with plan C. This function is called an appalling number of times (sheesh -- you'd think it would be cached) and I really don't see any other application. I would like to keep the name as I proposed because it means I could use module_implements with 'webform_menu_to_arg()' and still be backward compatible, should the unforeseen happen.

Committed to 7.x-4.x and 8.x.

Thank you for your help.