drupal_find_theme_templates() searches candidates that are never have templates and are duplicates [#2821420]

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

Comment	File	Size	Author
#6	2821420-6.patch	737 bytes	joelpittet
#6	8.4.x: PHP 5.6 & MySQL 5.5 21,449 pass
#4	2821420-1-reroll.patch	736 bytes	joelpittet
#4	8.4.x: PHP 5.6 & MySQL 5.5 21,470 pass
#2	2821420-1.patch	740 bytes	joelpittet
#2	8.3.x: PHP 5.5 & MySQL 5.5 Patch Failed to Apply

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

22 October 2016 at 22:52

joelpittet created an issue. See original summary.

Comment #2

joelpittet

English

Vancouver

CreditAttribution: joelpittet at The University of British Columbia commented 22 October 2016 at 22:56

Issue summary:	View changes
Status:	Active	» Needs review

File	Size
2821420-1.patch	740 bytes
8.3.x: PHP 5.5 & MySQL 5.5 Patch Failed to Apply

Comment #3

22 October 2016 at 22:56

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.

Comment #4

joelpittet

English

Vancouver

CreditAttribution: joelpittet at The University of British Columbia commented 8 March 2017 at 18:08

Assigned:

Unassigned

» lauriii

File	Size
2821420-1-reroll.patch	736 bytes
8.4.x: PHP 5.6 & MySQL 5.5 21,470 pass

1 file was hidden/shown/deleted

File	Size
2821420-1.patch	740 bytes
8.3.x: PHP 5.5 & MySQL 5.5 Patch Failed to Apply

Low-level and trivial patch. Rerolled.

Comment #5

lauriii

he/him

Finnish

Finland

CreditAttribution: lauriii as a volunteer commented 9 March 2017 at 05:29

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?

Comment #6

joelpittet

English

Vancouver

CreditAttribution: joelpittet at The University of British Columbia commented 10 March 2017 at 19:08

File	Size
2821420-6.patch	737 bytes
8.4.x: PHP 5.6 & MySQL 5.5 21,449 pass

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

Comment #7

lauriii

he/him

Finnish

Finland

CreditAttribution: lauriii as a volunteer commented 11 March 2017 at 05:14

Status:

Needs review

» Reviewed & tested by the community

Thanks for the change. This looks good now!

Comment #8

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Chapter Three commented 13 March 2017 at 16:32

Status:

Reviewed & tested by the community

» Fixed

Committed 7294785 and pushed to 8.4.x. Thanks!

Nice - this looks way more robust.

Comment #9

13 March 2017 at 16:33

alexpott committed 7294785 on 8.4.x

Issue #2821420 by joelpittet, lauriii: drupal_find_theme_templates()...

Comment #10

27 March 2017 at 16:34

Status:

Fixed

» Closed (fixed)

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

Comment #11

markhalliwell

English

CreditAttribution: markhalliwell at Tag1 Consulting commented 11 March 2018 at 18:02

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

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

Comment #12

joelpittet

English

Vancouver

CreditAttribution: joelpittet at The University of British Columbia commented 30 March 2018 at 04:55

@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.

Comment #13

markhalliwell

English

CreditAttribution: markhalliwell at Tag1 Consulting commented 30 March 2018 at 10:04

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.

Comment #14

joelpittet

English

Vancouver

CreditAttribution: joelpittet at The University of British Columbia commented 30 March 2018 at 21:06

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.

Comment #15

markhalliwell

English

CreditAttribution: markhalliwell at Tag1 Consulting commented 1 May 2018 at 17:36

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.