Turn hook names in theme() and module_invoke() into links

Would this feature also include linking the code "theme('foo', ...)" to the theme_foo page?
That would be a huge timesaver!

I had a look at the parser inc file and it baffled me... turning the string "theme('foo'" into a link is fairly simple, though obviously picking whether to link to the 'theme_foo' page or the preprocess function or the tpl file is an extra complexity -- but where would that code need to go?

Given how complex theme() is in Drupal 7 now that theme('links__node') is a standard way of calling theme()… a patch that did this would have to be quite complex.

marcvangend -- exactly

And anyway, it's really not rocket science -- this regex gets them in my D7 codebase:

Like I said above, it's a matter of knowing where to plug that into the code.

And just a note that it would need to link to either theme_foo() or foo.tpl.php, or perhaps something completely different, depending on hook_theme() implementations.

It would also be great if the theme_foo() or foo.tpl.php pages would list the calling function that called theme('foo') in the "Functions that call this function" section.

Here's an idea, in the form of a concrete summary of what I think would be a good and feasible outcome:

In code, you have a function call to:

'theme' would still be a link to the theme() function page.

'hook_name', 'alternate_hook_name', etc. would turn into links to the first of these that is found in the database:
- theme_hook_name() function
- hook-name.tpl.php file
In the case of the 'hookname__second__part' hook, we would look for:
- theme_hookname__second__part() or hookname--second--part.tpl.php
- theme_hookname__second() or hookname--second.tpl.php
- theme_hookname() or hookname.tpl.php
In the case of the array, we would do this process for hook_name first, then alternate_hook_name, etc.

In the case that no hook function/template could be found that matches, then we would not show a link.

It could be argued that we should try to do something more complicated -- hook_theme() has the ability to define totally unconnected names for the functions/templates. However, I think that case is rare and is probably mostly a bad idea (it leads to overly unreadable code). So let's not worry about that, and concentrate on this idea (which shouldn't be too impossible to implement I think?).

@jhodgdon: You've obviously got a grasp on how this module does its thing; any chance you could pad out its documentation so others can lend a hand with issues like this? I tried a while back and just couldn't understand how this operates.

RE #16 - we've had several new contributors to the API project recently who have figured it out (including myself)... if you have specific suggestions for what needs documentation, please file a separate issue or issues (and you might check the queue and see if any have already been filed).

Anyone who wants to help on this particular issue could look at the patch that was committed in #745204: Turn strings that are callback function names into function links as a starting point. I think the thing to do would be to add another argument to api_link_name() called $use_expanded, and if that is TRUE, in the if statement where it's checking to see if the text is an existing function, add another clause that checks for the existance of hook_$text, theme_$text, etc. It should be an easy change to make.

Jennifer, thanks for the where-to-start instructions. I'm not sure though why "use_expanded" would be a logical as argument name, but that's probably because I don't understand completely how api.module works.

Anyway, if someone wants to take this on, *please* assign to yourself as I'm in "fix a bunch of API issues" mode and I'll just do it probably sometime soon, unless I know someone else wants to.

I didn't have any trouble following the d.o docs the last time I set one up. If you do have trouble, please file a doc issue in this project, or if you find something that you think is obvious how to fix, edit the page. :)

marcvangend tried the idea above and reported to me in IRC that this is probably not the best idea. For instance:

The false positives on that other issue (where strings were turned into function links if the same exact name exists) were not excessive, but if we try to turn all strings xyz into hook_xyz or theme_xyz or theme.tpl.php links, we'll have a lot more false positives. So this will need to be done in a more careful way -- only turning them into links if we're in module_invoke(), theme(), etc.

I think I nailed it. This patch adds two search patterns to detect potential hook names inside theme(), module_invoke_all() and module_implements() calls. These search patterns use a 'positive lookbehind' regex. If one of the patterns is matched, api_link_name tries to find a matching function name after adding the 'hook_' or 'theme_' prefix to the name.

Unfortunately it's not possible to support module_invoke() in the same way, because module invoke has the $module variable as first argument before the hook name. A PCRE lookbehind can only match a fixed strings, but the $module value is passed in different ways throughout the code (eg. module_invoke($module, ....) and module_invoke($block->module, ...)). Still, I think the patch is worth committing without support for module_invoke.

Looking forward to your reviews.

It would be worth covering 'module_invoke($module, 'hook')' as a special case, since it's probably the most common one.

Is there a way to try the patch out by parsing just one file rather than the whole kaboodle?

It needed the comma and the space that come after the $module too.

I've broken the pattern up for legibility, which I think makes monster regexes much easier to figure out :)

Also:

> The code affected by this patch runs on every page request (not on cron), s

OMG, all these recursive regexes are run on every page view!? O_o

And also, I've filed this for later, as I really think this could do with a clean-up: #1364936: clean up scattered data in _api_link_documentation().

And beyond even that, I wonder if there's a way to process this differently so we don't need the lookbehinds...

Thanks!

Joachim: We don't break up regexps like this ANYWHERE else in core/contrib that I am aware of, and IMO it doesn't really help readability. Can you scrunch them back up? It just takes up a lot of space. Other than that, the patch in #32 looks like it follows our coding standards and conventions.

It also needs a test before I would commit it -- there are unit tests in the api_link_code.test file that can be used as a model -- just needs a few lines there added.

Also, to be complete, we need to link to template files as well as theme functions. E.g., if you call theme('block', ...), it should like to block.tpl.php, and if you call theme('search_result', ...) it should link to search-result.tpl.php. Note that _ has to be changed to - in template file names, so this will require yet another argument to that function. But that will allow us to get rid of the whole theme.php atrocity, such as this "gem" of documentation for a function that does not even really exist:
http://api.drupal.org/api/drupal/developer--theme.php/function/theme_sea...

I have never seen that done in Core [edit: Or, for that matter, elsewhere in the API module]. Please don't. Thanks!

Joachim, I really appreciate the work you're doing on this project and all other projects you have contributed to. (That's a long list on your profile page!) I understand your efforts to make regexes more readable and I do agree that your formatting is easier to understand. Meanwhile, I also respect and appreciate the fact that Jennifer wants us all to obey the Drupal coding standards. After all, they exist for a reason.

It would be a shame to see you go because of something like code formatting. Would it help you if we try to get some guidelines for regex formatting into the coding standards? That way, everybody wins.

OK... A line with just | on it doesn't make sense to me though. Can it be just a little more compact? Normally with arrays and long if() statements, we put the operator at the end of the line?

Please continue the discussion about regex formatting in #1365340: Decide on guidelines for regex formatting and let's return to improving api.module here.

Here's my latest patch.

I'm generating links to .tpl.php files now, using the pattern $template_name = str_replace('_', '-', $name) . '.tpl.php';. If there is a theme file for a theme hook (my-themehook.tpl.php), it takes precedence over the theme function (theme_my_themehook()).

For the time being, until there are guidelines telling me how to split long regexes, I'm sticking to single-line format.

Next on our todo list: tests.

I have only a little experience with writing tests, so while in theory I should be able to do it, I'm sure it will cost me a lot of time. If you don't mind writing the tests, I would love to review and test the patch afterwards, and learn a bit more about tests in the process. Thanks!

I found one thing. I have my PHP warning level set to the most strict possible, and I was getting a ton of:
Notice: Undefined variable: type in _api_link_documentation() (line 2197 of /home/jhodgdon/gitrepos/api/api.module).

That was easy to fix... just set $type = ''; before the switch($stage) in _api_link_documentation()

Anyway, just saying... it's always a good idea to develop with PHP strict turned on. Very enlightening.

Here, before I lose it, are some samples for testing this, to add to the above patch when it is all working...

While we're at it, let's make this work for drupal_alter() as well. I'll try to add that to the patch later today.

> RE #51: You mean you can't check for [a-zA-Z_]* instead of the literal "module" for the variable name? Hm. OK. We should change that in the samples then.

This is a limitation of the way the system works at the moment.
I'm not convinced it needs to have this whole recursive kaboodle, and that's what means we have to use a lookbehind, which is in turn what restricts us. Will file another issue on all that -- it's a future rewrite rather than something we can do now.

Completely agree with #54 and #55. A more advanced future version of this module could perhaps overcome the limitations of regex lookbehinds by some kind of hierarchy-aware (is that a word?) parsing, so when processing a string, you can access information about its parents. However that should not stop us from making a huge step forward right now.

All samples are working correctly now! The code theme('hookname__second_part') now links, in order of priority, to hookname--second-part.tpl.php, theme_hookname__second_part(), hookname.tpl.php or theme_hookname(). The non-existent functions were not shown because the fall-back option (return the name without a link) was wrapped in an else{} block. I simply took out the else statement because the fall-back should always run if nothing has been returned yet.

The third case from #14, theme(array('hook_name', 'alternate_hook_name'), ...);, is not covered by this patch. It cannot be done properly because of the regex limitations mentioned before. In this example, we could turn "hook_name" into a link because the lookbehind "theme(array(" is a known, fixed-length string. However we cannot turn "alternate_hook_name" into a link because the value of the first hook name is unknown. It's also worth noting that I have not found a single occurrence of this syntax (outside in-line docs and automated tests) in Drupal 7 core.

Assuming the status on this should now be "needs review"... I'll test it out ... probably not until tomorrow though. Also needs tests to be added before it can be committed, but as we discussed above, I'm willing to take care of the tests.

In reply to #57- the reason this is like this is that this code is like this is that we want each section of text to only be processed once. If something has been potentially made into a link, we don't want to process it again. For example, if something is in @link/@endlink, we don't want other stuff to even attempt dealing with it. The unmatched text is passed recursively to try out the next levels. Other ways of tokenizing might be better, especially for code, but that's another issue.

I think this can be done without removing that last else() if you do this. You have, e.g.:

Just change it to:

Right?

I'd also suggest changing this comment:

Anyway, the patch appears to function as desired. So can you make those changes, and then I'll make some tests and get it committed?

I'm trying to understand why the removal of the else{} statement bothers you. I was never officially trained as a programmer, but as far as I'm concerned, there is nothing wrong with this pattern:

You're right that the $type == 'hook' part can be easily reduced to a single if-statement instead of two nested if-statements, but it's not that easy once you get to the $type == 'theme' block, because it has a while-loop inside.

If you insist on bringing back the else{} block, I guess we would have to split off the stuff inside the $type == 'theme' into a separate helper function (eg. _api_match_theme_hook()) and do something like this (untested code, but I think you get the idea):

And no, no need to try to munge the theme part into a single if().

Ok, thanks for the feedback, I'll correct the comment later today.

You're right that we do need the former else-bit to run whenever there has not been a return earlier, so that was the reason to remove the else{}. I tried to explain that in the "// Fallback: ..." comment, but it seems that isn't clear enough, otherwise we wouldn't been having the discussion above. Do you have any suggestions how we can improve that comment?

Here's a new patch with an improved comment for the 'code hook name' regex. I altered your suggested comment a little bit because I also wanted to cause of the restriction. I ended up with:

My comment #70 crossed your #69, but thanks for the tip. I made the comment before the fallback part a bit more verbose:

I'm also adding a second patch because I found the actual code for the fallback longer than necessary. The current code is:

Awesome, thanks. It's nice to see that I've learned enough the past couple of years to fix my own feature requests :-) I'll have a look at the rest of the API queue.

By the way, do you have any idea when this will reach api.drupal.org?

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

See also #1394404: On a hook, theme hook, or theme template page, list functions that invoke this hook/theme which is doing similar things at parse time.

Great! Bonus points if you add tests to the link_documentation tests -- you'll need to add to the samples to get those to work.

I also was thinking that many of the *invoke functions work differently in D6 and D7 -- the names of the hooks they are invoking changed. So we probably need this new code to look for any possible hook that might exist (d6 of d7 style) in the current branch that would be invoked.

And if this is too complicated or won't work, we can also just set this back to "fixed" and decide these custom invoke functions are not worth doing. Or just do some. Or only support the D7 hook names. I'll leave it up to you to figure out. :)

I think we should postpone this until we figure out if #1405914: Change code link/reference creation to use Grammar Parser can address it.