To reproduce:
Define a path (through hook_menu or another module (eg views)) with a hyphen.
Eg 

/**
 * Implements hook_menu
 */
function mymodule_menu() {
 $items['my-node'] = array(
    'page callback' => 'node_page_default',
    'access arguments' => array('access content'),
    // Required to make 'node/add' appear on the top-level of 'navigation' menu.
    'menu_name' => '',
    'type' => MENU_CALLBACK,
  );

  return $items;
}

*Make the following page template in your theme

page--my-node.tpl.php

*Flush your caches
*The new template is not used because:
**The theme registry auto-discovery mechanism replaces all hyphens in the templates it finds with underscores - so in this case the theme_registry has a hook for page__my_node
**but theme_get_suggestions does not do the same thing in reverse so if the args come in as my-node, the theme_hook_suggestions array contains page__my-node - for which there is no matching theme hook

This patch changes theme_get_suggestions to replace hyphens with underscores so that the suggestions returned match the hooks registered and hence the template is used.

CommentFileSizeAuthor
#37 HyphenPathsAndThemeHookSuggestions-992376-30.patch900 byteslarowlan
#30 HyphenPathsAndThemeHookSuggestions-992376-30.patch900 byteslarowlan
#27 HyphenPathsAndThemeHookSuggestions-992376-27.patch1.82 KBlarowlan
#24 HyphenPathsAndThemeHookSuggestions-992376-24.patch1.65 KBlarowlan
#22 HyphenPathsAndThemeHookSuggestions-992376-22.patch1.65 KBlarowlan
#20 HyphenPathsAndThemeHookSuggestions-992376-18.patch1.52 KBlarowlan
#15 HyphenPathsAndThemeGetSuggestions-992376-15.patch1.52 KBlarowlan
#12 issue-992376.patch1.63 KBlarowlan
#6 issue-992376.patch1.51 KBlarowlan
#3 mymodule.tar_.gz734 byteslarowlan
#3 page--my-module--node.tpl_.php_.txt6.72 KBlarowlan
#2 issue-992376.patch872 byteslarowlan
#1 issue-992376.patch892 byteslarowlan
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
Status: Active » Needs review
FileSize
issue-992376.patch892 bytes

Here is the patch

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
Version: 7.0-rc1 » 7.0-rc2
FileSize
issue-992376.patch872 bytes

Reroll against RC2

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
FileSize
page--my-module--node.tpl_.php_.txt6.72 KB
mymodule.tar_.gz734 bytes

Attached is
a) a module for testing this, it defines a path my-module/node which is just the default /node callback
b) a page template for this path (page--my-module--node.tpl.php) which goes in your active theme folder

To reproduce
1) Copy the module to the appropriate place
2) Copy the page tpl to your theme folder and remove the .txt extension
3) Enable the module
4) Flush your cache (to be sure)
5) Visit /my-module/node

Without the patch - you notice no difference - the new template is not recognised.
With the patch - you get 'THIS IS THE ALTERNATE PAGE TEMPLATE' in large letters at the top of the page

realityloop’s picture

realityloop
he/him
CreditAttribution: realityloop commented
Status: Needs review » Reviewed & tested by the community

Tested both with and without patch and confirmed working with #2 patch

Dries’s picture

Dries CreditAttribution: Dries commented 
+++ includes/theme.inc	12 Dec 2010 21:45:59 -0000
@@ -2365,7 +2365,8 @@ function theme_get_suggestions($args, $b
+    // Change - to _ as all underscores in theme registry hooks are converted to hyphens

This comment reads as if we're doing something dumb. First we convert - to _, then we convert _ to -. It sounds like we want to be more articulate.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
FileSize
issue-992376.patch1.51 KB

Here it is again with revised comments

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
Status: Reviewed & tested by the community » Needs review
realityloop’s picture

realityloop
he/him
CreditAttribution: realityloop commented
Status: Needs review » Reviewed & tested by the community

Updated comment is easier to read.

realityloop’s picture

realityloop
he/him
CreditAttribution: realityloop commented
Version: 7.0-rc2 » 7.x-dev

Bumping version so it doesn't get missed

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Well, that's some commenting! :)

I made a couple of small formatting adjustments (moved note about hyphen/underscore to previous line, removed the link to this issue since we don't do that really, added a period, and Americanized the word "recognized") and...

Committed #6 (with modifications) to HEAD.

It'd be nice if it's possible to have a testcase for this. Seems like we ought to be able to. So marking down to "needs work" for that.

mdupont’s picture

mdupont
Primary language French
Location Switzerland
CreditAttribution: mdupont commented

It looks like issues with hyphens and template suggestions aren't limited to pages... See #1021270: Blocks for custom menus are impossible to theme for such a bug with the blocks created by menu.module. No custom menu block can use specific templates.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
Status: Needs work » Needs review
FileSize
issue-992376.patch1.63 KB

Here's a patch for review

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
Status: Needs review » Needs work

sorry, wrong issue :-(

pixelite’s picture

pixelite
Primary language English
Location Montreal
CreditAttribution: pixelite commented

Just to clarify comment 13... larowlan meant to post his patch in comment 12 to #1021270

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
Version: 7.x-dev » 8.x-dev
Status: Needs work » Needs review
Issue tags: +Needs backport to D7
FileSize
HyphenPathsAndThemeGetSuggestions-992376-15.patch1.52 KB

Here's a patch that adds a test case.
Thanks wojtha for pointing me in the right direction for this.
Updating to D8 for fixing there first.
Tagging to add 'needs backport'.

Status: Needs review » Needs work

The last submitted patch, HyphenPathsAndThemeHookSuggestions-992376-15.patch, failed testing.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
Status: Needs work » Needs review

Hmm passes locally against D7 master, time to get a D8 environment up and going

realityloop’s picture

realityloop
he/him
CreditAttribution: realityloop commented

not sure why.. but #6 already appears to be committed to D8

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
Status: Needs review » Needs work

Yes, #6 made it into 7.0

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
Status: Needs work » Needs review
FileSize
HyphenPathsAndThemeHookSuggestions-992376-18.patch1.52 KB

Hmm passes on local D8 install too, trying again in case something dodgy with the previous patch

Status: Needs review » Needs work

The last submitted patch, HyphenPathsAndThemeHookSuggestions-992376-18.patch, failed testing.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
Status: Needs work » Needs review
FileSize
HyphenPathsAndThemeHookSuggestions-992376-22.patch1.65 KB

Adding more debug to tests

Status: Needs review » Needs work

The last submitted patch, HyphenPathsAndThemeHookSuggestions-992376-22.patch, failed testing.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
Status: Needs work » Needs review
FileSize
HyphenPathsAndThemeHookSuggestions-992376-24.patch1.65 KB

More test debugging in this patch

Status: Needs review » Needs work

The last submitted patch, HyphenPathsAndThemeHookSuggestions-992376-24.patch, failed testing.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented

Ok, so testbot test is failing because drupal_is_front_page evaluates true for the test bot, not for local simpletest.
Will add additional logic to the test.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
Status: Needs work » Needs review
FileSize
HyphenPathsAndThemeHookSuggestions-992376-27.patch1.82 KB

Hopefully this one is the last (apologies to all those who've subscribed via email to this thread).

clemens.tolboom’s picture

clemens.tolboom
Primary language Dutch
Location Groningen, 🇳🇱/🇪🇺
CreditAttribution: clemens.tolboom commented
Status: Needs review » Needs work 
+++ b/modules/system/system.test
@@ -2149,3 +2149,42 @@ class SystemAuthorizeCase extends DrupalWebTestCase {
+      'description' => 'Test the template_preprocess_page() function returns
+                        appropriate template suggestions.',

Why is this 'space' layout-ed?

+++ b/modules/system/system.test
@@ -2149,3 +2149,42 @@ class SystemAuthorizeCase extends DrupalWebTestCase {
+    // Testbot on Drupal.org runs in context of front page.
+    if (drupal_is_front_page()) {
+      $result[] = 'page__front';
+    }

Is this a bug of our testbot?

Our should you navigate to the right context somehow?

Powered by Dreditor.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented

Will add a hook_menu definition with the hyphen path and then navigate to that path before running the test.
Should be able to use the arg function then.
Thanks for the review

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
Status: Needs work » Needs review
FileSize
HyphenPathsAndThemeHookSuggestions-992376-30.patch900 bytes

Ok, moved logic to theme.test where similar tests were being performed.
Cleaned up comments.

dcrocks’s picture

dcrocks CreditAttribution: dcrocks commented

There are 3 other related issues with similar patches:
http://drupal.org/node/1021270
http://drupal.org/node/1129972
http://drupal.org/node/1076132

And shouldn't this issue be under the block module component?

Status: Needs review » Needs work

The last submitted patch, HyphenPathsAndThemeHookSuggestions-992376-30.patch, failed testing.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
Status: Needs work » Needs review

dcrocks, this issue concerns page templates, not blocks, see #1021270: Blocks for custom menus are impossible to theme for the block issue.
For what it's worth the patch for this issue made it into Drupal 7.0, we are just working on tests now.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented

#30: HyphenPathsAndThemeHookSuggestions-992376-30.patch queued for re-testing.

dcrocks’s picture

dcrocks CreditAttribution: dcrocks commented

Sorry, patch in #12 confused me.

404’s picture

404 CreditAttribution: 404 commented

subscribe

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
FileSize
HyphenPathsAndThemeHookSuggestions-992376-30.patch900 bytes

For some reason the patch at 30 failed the first bot test, but passed the second one - reattaching here to clear up all confusion

realityloop’s picture

realityloop
he/him
CreditAttribution: realityloop commented
Status: Needs review » Reviewed & tested by the community

Passed locally too:

Pass Other theme.test 49
Found expected page suggestions for paths containing hyphens.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Awesome! Thanks a lot for the tests!

Committed to 8.x and 7.x!

clemens.tolboom’s picture

clemens.tolboom
Primary language Dutch
Location Groningen, 🇳🇱/🇪🇺
CreditAttribution: clemens.tolboom commented

Do we 'needs backport to D7' still? #39 displays -needs backport to D7 _AND_ +needs backport to D7 ?!?

According to http://drupalcode.org/project/drupal.git/commit/e85b057 this is committed to D7

Do we have a tags workflow guide somewhere?

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented

I like leaving the tag there so I can go back and see what I've fixed.

I have no idea why it's resetting the tags randomly though. Must be some bug in comment_alter_taxonomy.

Status: Fixed » Closed (fixed)
Issue tags: -Needs tests, -Needs backport to D7

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

Issue tags: +Needs tests, +Needs backport to D7

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.