Relegate lists of function calls and hook implementations to separate pages

Seems reasonable... Would need a patch, including that some tests would need to be tweaked (there are tests that look for links that are in the "functions that call this" section, and they would need to click the link to the other page, and look there instead).

Thanks for taking this on! (you might assign the issue to yourself too).

I took a quick look through your patch, and it looks really good (haven't tested it yet). A few things that might need attention:

a)

+    $replaced_string = str_replace(API_FILEPATH_SEPARATOR, API_FILEPATH_SEPARATOR_REPLACEMENT, $function->file_name);
+    $call_path = 'api/'. $branches[$function->branch_id]->project .'/'. $replaced_string .'/function/calls/'. $function->object_name .'/'. $branches[$function->branch_id]->branch_name;

This should probably be refactored into a function, as very similar code is being used below to build the link to the hook implementations too.

b) What's this?

--- a/templates/api-functions.tpl.php
+++ b/templates/api-functions.tpl.php
@@ -10,6 +10,7 @@
  * - $function['description']: Function description.
  */
 ?>
+<h2><?php print $title ?></h2>

It doesn't seem related to this patch?

c) Nitpick:

+ * - $title: The title of the link e.g., "N functions call function()".

e.g. needs to be preceded by ;

Thanks!

Thanks for the new patch!

RE #3/#4 c) -- it's a punctuation nitpick. When you use "e.g.", which means "for example", in this way, it needs a semi-colon before it.

RE #3/#4 b) But the $title variable looks like it was added to an existing template, not your new template? ... Oh I see, you are using that existing template for a new purpose. Anyway, yes, I think that should definitely be the page title, not some additional text added to the page. And there should be a link back to the main function page as well, shouldn't there?

Looking at the new patch...
1) I think the new theme function should probably go into the api.module file rather than in this new theme.inc file. Or better yet, maybe all of the page-generating functions from this module should go into api.pages.inc, which is really the standard in core and other Drupal modules, rather than being in the main module file? If that is too much to do for this patch, let's file another issue, and for the moment move this new function back into the main api.module file.

2) This patch still needs updates so that the existing tests will pass, and new tests for this new functionality.

Filed:
#1414490: Move page-generating functions into include file, and refactor repeated code

Regarding the page title -- I don't think "because it's annoying to do" is a good reason not to have the correct page title on the page. :) Keep in mind that page titles show up in Google searches, and we wouldn't really want someone to go to the "functions that call xyz()" page when they really wanted the "function xyz()" page, would we? Also they are what show up in your browser's title bar or tab. We should have them accurate. I think the page title should be:
Functions calling xyz()
Functions invoking hook_xyz()
Or something similar.

Regarding a link back in the breadcrumb: breadcrumbs are nice, but not every site/theme chooses to display breadcrumbs. How about in the line that says "263 functions call xyz()", we make xyz() be a link back to the xyz() page?

Sorry, I interpreted #7 (b) as "the way things are set up now, this would be annoying/difficult to do". Which I think means that the way things are set up now should be refactored? I didn't mean to offend! I really appreciate you working on this!!

I'm not sure what to say about the breadcrumb/page title/link back to the function... I think the link should be somewhere on the page and not just in the breadcrumb though. Perhaps the whole thing should be navigation/hook_menu() tabs that would let you switch between the function, hooks that invoke it, and/or functions that call it? No... probably not... Not sure what to suggest...

All right, that's probably OK. We display breadcrumbs on api.drupal.org too. :)

So... Do you want me to take over on the test fix-up? Since I wrote the tests that are likely failing pretty recently, that might make sense. Is that all you think needs to be done?

Sounds good!

So I haven't applied your patch and run the tests yet... but I imagine that most of the tests that are failing are in api/tests/api_web_test.test in ApiSimpleItemPagesTestCase::testFunctionPage().

These tests are set up to work with the api/tests/sample directory as a branch. So on your API test site, you can set that up as a branch to see what its pages look like, to aid in writing tests.

Then you can probably follow the logic of that test. It visits several function pages by calling $this->drupalGet(), and verifies that certain text/links are on the page. For instance when it visits the duplicate_function() page [first ->drupalGet() in the test function], it does:

    $this->assertText('1 function calls duplicate_function()', 'List of calling functions is present');
    $this->assertLink('sample_in_code_links', 0, 'Calling function link is present');
    $this->assertText('Does something interesting, to test', 'Calling function description is present');

That first assert is probably fine (although the message should change to "Link to calling functions page is present"), since I think this link text hasn't changed, right? But those other two asserts are not going to work, because these are testing that the calling functions are being displayed, and they aren't on the same page now.

So what you'd need to do is take those two lines out, and farther down at the end of the section that says "Click the automatically-generated ...", you would want to do something like this [which I haven't tried but should mostly work]:

    $this->drupalGet('api/' . $this->branch_info['project'] . '/sample.php/function/duplicate_function');
    $this->clickLink('1 function calls duplicate_function()');
    $this->assertTrue(strpos($this->url, $this->branch_info['project'] . '/sample.php' ... [whatever the URL should be here]) !== FALSE, 'Calling functions link went to the right place');
    $this->assertLink('sample_in_code_links', 0, 'Calling function link is present');
    $this->assertText('Does something interesting, to test', 'Calling function description is present');

The first line takes you back to the duplicate_function() page.
The second line follows the link to the calling function page.
The third line verifies you landed on the right URL.
The fourth and fifth lines I took from the failed assertions above. You might also want to verify that there is a link back to the duplicate_function() page [from the breadcrumb].

Does that help?

I have never seen that before. Maybe the api.drush.ws site has a patch that they haven't contributed to the API module?

Excellent, thanks! I'll give it a run-through in the next day or two. It won't get committed until after the next release though, since we're in "beta" phase and I don't want to introduce a bunch of new code while we're testing for a release/api.drupal.org deploy.

I tested the patch today -- it works fine, and all the tests pass.

I also took a close look at the code in the patch, and I have a couple of ideas/concerns:

a)

-function api_object_title_and_breadcrumb($branch, $object) {
-  drupal_set_title($object->title);
+function api_object_title_and_breadcrumb($branch, $object, $title = NULL) {
+  // Allow the title to be overridden.

...
-  $page_title[] = check_plain($object->title);
+  // Set the page title and breadcrumb for the function calls and
+  // implementations pages.
+  $menu_item = menu_get_item();
+  if ($menu_item['path'] == 'api/%/%/function/calls/%' ||
+      $menu_item['path'] == 'api/%/%/function/implementations/%') {
+    $page_title[] = $title;
+    $breadcrumb[] = l($object->object_name, api_url($object));
+  }
+  else {
+    $page_title[] = check_plain($object->title);
+  }

It seems like this if($menu_item['path']) ... stuff would be better off as a new Boolean (default FALSE) argument called $object_name_in_breadcrumb or something like that? You already added one new function argument to this function, and this seems like a good place to use an argument rather than relying on the menu system paths?

b)

+function api_page_function_implementations($function) {
+  $branches = api_get_branches();
+  $branch = $branches[$function->branch_id];
+
+  // Special case: If this is a hook, make a list of functions that appear to
+  // be implementations of this hook.
+  if (strpos($function->title, 'hook_') === 0) {

The in-code comment (taken from where this code used to be) probably shouldn't say "special case" because now this is what this function does?

And would a better name for this function be api_page_hook_implementations()?

c) What happens in function api_page_function_calls() and api_page_function_implementations() when the count is zero? Hmmm. I can answer that. I didn't get any error messages to a function/calls path for a function without any calling functions -- just a fairly empty page with title "0 functions call ...". That's OK. But the function/implementations page gave me two PHP notices:

Notice: Undefined variable: hook_functions in api_page_function_implementations() (line 1935 of /home/jhodgdon/gitrepos/api/api.module)
Notice: Undefined variable: hook_functions in api_page_function_implementations() (line 1939 of /home/jhodgdon/gitrepos/api/api.module).

I'm not sure that's important to fix? But it isn't difficult to fix.

d) The in-code comments in api_page_function() that say:

  // Build the list of functions that call this one, from the records added
  ...
and
  // Special case: If this is a hook, make a list of functions that appear to be
  // implementations of this hook.

These comments are not correct any more. All that's being built is the link to the separate pages. Needs updating.

e) I don't think this is really internationalized correctly:

+  $action = ($type == 'calls') ? format_plural($count, 'calls', 'call') : format_plural($count, 'implements', 'implement');
+  $link_title = format_plural($count, '1 function @action @name()', '@count functions @action @name()', array('@action' => $action, '@name' => $function->title));

I think it would be better to just make two separate calls here, because some languages have really complicated verb forms (not just calls/call and implement/implements for single vs. plural). So this won't be translatable for all languages. Instead we should have something like this:

if ($type == 'calls') {
   $link_tltle = format_plural($count, '1 function calls @name()', '@count functions call @name()', [etc.]);
}
else {
   ...
}

f) Maybe we need a test for the "N functions implement hook_foo()" page too? It would require adding a function foo_sample_name() to the sample code [which would be an "implementation" of hook_sample_name() that is already there]. And as a note, if you do add a function to the sample code, there are a couple of tests that will fail in the "Branch and object creation" tests -- they count the number of objects that are created during parsing, so the numbers have to get incremented.

forgot status

Looks excellent to me! The only thing I would add is in the test in this section:

+    // Test the function implementations link and page.
+    $this->drupalGet('api/' . $this->branch_info['project'] . '/sample.php/function/hook_sample_name');
+    $this->assertLink('1 function implements hook_sample_name()', 0, 'Link to implemented hooks page is present');
+    $this->clickLink('1 function implements hook_sample_name()');
+    $this->assertTrue(strpos($this->url, $this->branch_info['project'] . '/sample.php/function/implementations/hook_sample_name') !== FALSE, 'Link to implementing functions page went to the right place');
+    $this->assertText('1 function implements hook_sample_name()', 'Title of implementations listing page is correct');

Maybe at the end of this add an assertion for a link to foo_sample_name to verify that this function link appears on the page, and an assertion that it's one-line description text is there?

And if you are fixing this, there is one other extremely minor thing:

+    $hook_count = $hook_count - 1;

Could use $hook_count-- here. I probably originated that code -- my bad. :)

I'll make time to check this out sometime this week. THANKS again!

This patch looks good!

The only thing I noticed this time is that I'm not convinced we need the $branches argument to:

theme_api_function_reference_link($type, $count, $branches, $function) {

because the api_get_branches() function is cheap to call (static cached), and it's kind of background information rather than information that's specific to making that link. But I can fix this on the commit pretty easily.

Anyway, the patch applies fine, the tests pass, and the pages seem to be working fine (after clearing the cache anyway). Hopefully we'll get out of beta-testing mode sometime soon, and I'll be able to commit some of these RTBC patches.

Heads up! I just found #1426106: List of functions calling this function includes other branches in the existing code; that fix will likely need to get into this patch as well, since I think this uses the same DB query.

I need to fix that other issue before the next release, like right now, and I'm not committing this one until after the release.

After that fix, I imagine this patch won't apply since the lines I need to change are taken out by this patch. Also, I wouldn't want to update the query on that other issue, and then have this patch use the old query that is listing functions from other branches.

Sorry! Yeah, it was pretty blatant on the staging site. We can't release/deploy with the code like that. :)

The needed code and test changes have been committed. This should be OK to work on again. I apologize in advance for any refactoring/rebasing you may encounter... and I hope it goes smoothly...

I have unfortunately made some more commits and the code is different again... I think I can get this patch in though, with a little manual work. Tenatively assigning to me to get it committed.

One note: we are using the ctools collapse stuff for the View Source on File pages, so I don't think we can completely eliminate it.

This is committed:
http://drupalcode.org/project/api.git/commit/f6ceb81
Thanks psynaptic!