Follow-up to #2817103: Theme registry doesn't find templates in subfolders for self-defined theme hooks

Problem/Motivation

Noticed $template_candidates uses $info['theme path'] which never shows a different template.

Proposed resolution

Remove candidates

CommentFileSizeAuthor
#6 2821420-6.patch737 bytesjoelpittet
#4 2821420-1-reroll.patch736 bytesjoelpittet
#2 2821420-1.patch740 bytesjoelpittet

Comments

joelpittet created an issue. See original summary.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new 2821420-1.patch740 bytes

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Assigned: Unassigned » lauriii
StatusFileSize
new 2821420-1-reroll.patch736 bytes

Low-level and trivial patch. Rerolled.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Assigned: lauriii » Unassigned

I spent quite a long time figuring this out, testing multiple different stages of Drupal 8. I can confirm that this is not needed anymore. This was added as part of #342350: Override template files based on template filename as well as hook name but I can confirm that that use case doesn't need this bit of code anymore. There's also tests proving that this use case still works.

I also searched for the reason why this became redundant. It was caused by this issue #2226207: Make 'template' the default output option for hook_theme() where we separated paths from the template names.

+++ b/core/includes/theme.inc
@@ -240,8 +240,7 @@ function drupal_find_theme_templates($cache, $extension, $path) {
+        if ($template == $info['template']) {

Should we use === here?

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
StatusFileSize
new 2821420-6.patch737 bytes

I think we should use ===, you're right because of those silly edge cases:
php -r "var_dump(0 == 'a'); " => TRUE

Thank you for the review

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Needs review » Reviewed & tested by the community

Thanks for the change. This looks good now!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 7294785 and pushed to 8.4.x. Thanks!

Nice - this looks way more robust.

  • alexpott committed 7294785 on 8.4.x 
    Issue #2821420 by joelpittet, lauriii: drupal_find_theme_templates()...

Status: Fixed » Closed (fixed)

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

markhalliwell’s picture

markhalliwell
Primary language English
commented

Well, this change certainly snuck its way in...

Here's a change record (which this should have had):
https://www.drupal.org/node/2952033

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

@markcarver, I think you may have misinterpreted the patch.

Before:
[$info['template'], str_replace($info['theme path'] . '/templates/', '', $info['template'])];

in_array() checks both templates: one as a whole and one without the theme path + templates

After:
Only
$info['template'] === $template

The rub is that $info['template'] never has the path in it. So the second check is the same as the first always.

Here's a sample of the output of both variables separated by a colon:

blazy:blazy
block:block
block-content-add-list:block-content-add-list
calendar-mini:calendar-mini
calendar-month-multiple-entity:calendar-month-multiple-entity
calendar-datebox:calendar-datebox
calendar-empty-day:calendar-empty-day
calendar-month-col:calendar-month-col
calendar-month-row:calendar-month-row
calendar-day:calendar-day
calendar-month:calendar-month
calendar-stripe-legend:calendar-stripe-legend
calendar-item:calendar-item
calendar-week:calendar-week
calendar-week-overlap:calendar-week-overlap
calendar-day-overlap:calendar-day-overlap
calendar-year:calendar-year
calendar-header:calendar-header
calendar-time-row-heading:calendar-time-row-heading
calendar-pager:calendar-pager
ckeditor-settings-toolbar:ckeditor-settings-toolbar
conditional_field:conditional_field
conditional-field-content-add-list:conditional-field-content-add-list
entity-moderation-form:entity-moderation-form
contribute-status-report-community-info:contribute-status-report-community-info
crop-crop-summary:crop-crop-summary
ctools-wizard-trail:ctools-wizard-trail
ctools-wizard-trail-links:ctools-wizard-trail-links
date-recur-default-formatter:date-recur-default-formatter
dropzonejs:dropzonejs
html--entity-browser--iframe:html--entity-browser--iframe
html--entity-browser--iframe:html--entity-browser--iframe
page--entity-browser--iframe:page--entity-browser--iframe

I'm going to unpublish your CR for now so people don't get confused. Hope you don't mind, please correct me if I made a mistake though.

markhalliwell’s picture

markhalliwell
Primary language English
commented
Related issues: +#2951575: Templates limited to "templates" directory of sub-theme

Intentional or not, this suddenly "opened up" template discovery to anywhere in the theme when it was previously limited to just the /templates directory.

I had to adjust the Drupal Bootstrap base theme accordingly.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

I don't think it did though... how? did you read the explanation on what it does and how this changed nothing from what I can see, maybe I'm not seeing what you are seeing, please explain.

markhalliwell’s picture

markhalliwell
Primary language English
commented

After re-evaluating this and what @joelpittet mentioned, I'm not convinced this does what I originally said it did.

The registry still defaults to the /templates directory of a theme:
http://cgit.drupalcode.org/drupal/tree/core/lib/Drupal/Core/Theme/Regist...

// Prepend the current theming path when none is set. This is required
// for the default theme engine to know where the template lives.
if (isset($result[$hook]['template']) && !isset($info['path'])) {
  $result[$hook]['path'] = $path . '/templates';
}

I think I got confused between $info['path'] (which is for the template) and $info['theme path'] which is the root directory of the theme (and what drupal_find_theme_templates() was using).

I've deleted the change record.