API page: http://api.drupal.org/api/drupal/developer--hooks--core.php/function/hoo...

The attached code on the "hook_theme_registry_alter" documentation page, under the context "developer/hooks/core.php, line 2205" has either a typo or a rather illogical if statement in it's foreach.

$value = 'template_preprocess_forum_topic_navigation'

Which probably should be:

$value == 'template_preprocess_forum_topic_navigation'
Files: 
CommentFileSizeAuthor
#13 hook_theme_registry_alter-1312178-13.patch680 bytesrocket_nova
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch hook_theme_registry_alter-1312178-13.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]
#10 hook_theme_registry_alter-1312178-3.patch700 bytesjhodgdon
PASSED: [[SimpleTest]]: [MySQL] 36,851 pass(es).
[ View ]
#8 hook_theme_registry_alter-1312178-8.patch995 bytesrocket_nova
PASSED: [[SimpleTest]]: [MySQL] 33,648 pass(es).
[ View ]
#3 hook_theme_registry_alter-1312178-3.patch700 bytesrocket_nova
PASSED: [[SimpleTest]]: [MySQL] 33,628 pass(es).
[ View ]

Comments

jhodgdon’s picture

Version:7.x-dev» 8.x-dev
Issue tags:+Novice, +needs backport to D7

Good catch! Good project for a novice patch contributor to fix.

rocket_nova’s picture

Assigned:Unassigned» rocket_nova

Working on this at BADCamp.

rocket_nova’s picture

Status:Active» Needs review
StatusFileSize
new700 bytes
PASSED: [[SimpleTest]]: [MySQL] 33,628 pass(es).
[ View ]

Patch attached!

Also the Drupal 8 API pate is at: http://api.drupal.org/api/drupal/modules--system--system.api.php/functio...

rocket_nova’s picture

Should this also be tagged for backport to D6 since the error is appearing in the D6 API also?

jhodgdon’s picture

Status:Needs review» Reviewed & tested by the community
Issue tags:+needs backport to D6, +Quick fix

Thanks for the patch, looks good -- and yes it needs to be tagged for d6 backport.

catch’s picture

Version:8.x-dev» 7.x-dev

Thanks! Committed and pushed to 8.x, moving to 7.x.

Alan D.’s picture

Followup from #1316774: Documentation problem with hook_theme_registry_alter, this code assumes that the forum module is installed. If it isn't, then the code will generate errors. How pedantic should we be about these code examples? This is much less readable.

ie: A real life example would be:

<?php
function hook_theme_registry_alter(&$theme_registry) {
 
// Kill the next/previous forum topic navigation links.
 
if (!empty($theme_registry['forum_topic_navigation']['preprocess functions'])) {
    foreach (
$theme_registry['forum_topic_navigation']['preprocess functions'] as $key => $value) {
      if (
$value == 'template_preprocess_forum_topic_navigation') {
        unset(
$theme_registry['forum_topic_navigation']['preprocess functions'][$key]);
      }
    }
  }
}
?>
rocket_nova’s picture

Version:7.x-dev» 8.x-dev
Status:Reviewed & tested by the community» Needs review
StatusFileSize
new995 bytes
PASSED: [[SimpleTest]]: [MySQL] 33,648 pass(es).
[ View ]

Ok @Alan D, I think that makes sense. Here is a re-rolled patch for 8.x with the if check.

catch’s picture

Version:8.x-dev» 7.x-dev
Status:Needs review» Reviewed & tested by the community

That change looks fine but please move this to another issue so we don't need to version ping ping this one.

jhodgdon’s picture

StatusFileSize
new700 bytes
PASSED: [[SimpleTest]]: [MySQL] 36,851 pass(es).
[ View ]

Note: Ignore the patch in #8. I'm reattaching the 7.x patch that is the one that is RTBC for 7.x.

We have lots of hook examples that use various core modules that may or may not be enabled on any given site. They are only meant to be models, so this is OK.

webchick’s picture

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 7.x. Great find! :)

jhodgdon’s picture

Version:7.x-dev» 6.x-dev
Status:Fixed» Patch (to be ported)
Issue tags:-needs backport to D7

This one's marked backport to d6.

rocket_nova’s picture

Status:Patch (to be ported)» Needs review
StatusFileSize
new680 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch hook_theme_registry_alter-1312178-13.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

Here's the D6 patch for review.

Status:Needs review» Needs work

The last submitted patch, hook_theme_registry_alter-1312178-13.patch, failed testing.

jhodgdon’s picture

Project:Drupal core» Documentation
Version:6.x-dev»
Component:documentation» API documentation files

Sorry! I forgot to move this into the Documentation project. The d6 hook docs live in this project's git repository. The patch needs to be rerolled.

sven.lauer’s picture

Status:Needs work» Reviewed & tested by the community

The patch in #13 is actually fine---it was rolled against the Documentation repository. The testbot got triggered because the issue itself was still set to core.

The patch applies fine and does the right thing (and is fairly minimal), RTBCing.

jhodgdon’s picture

Status:Reviewed & tested by the community» Fixed

committed and pushed.

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