Improve documentation of hook_theme_suggestions_HOOK_alter() [#3343198]

Problem/Motivation

Themers & front-end developers are not given enough information to know exactly why new template suggestions may fail. As discussed in slack with @safetypin, consider the following

THEME.theme

function microsite_theme_suggestions_page_alter(array &$suggestions, array $variables) {
  $suggestions[] = 'microsite_page';
}

This adds a new item to the THEME_DEBUG comments in the markup when twig.config.debug === true, but that template will not be used.

 FILE NAME SUGGESTIONS:
   * microsite-page.html.twig
   * page--photocontest.html.twig
   * page--group--371.html.twig
   * page--group--%.html.twig
   x page--group.html.twig
   * page.html.twig

While this suggestion was added correctly, and appears in the theme debug, it's not a valid template name because it does not start with hook__, as expected in drupal_find_theme_templates() (source).

Steps to reproduce

In a custom theme, attempt to add a new theme suggestion that does not begin with the theme hook and two underscores
Enable twig debug and note that the invalid name appears as a theme suggestion
Create the matching twig file and note that it will not be used, even though a suggestion exists for it

Proposed resolution

There are a couple things that I think we ought to do to make this better:

We need to add this rule to the api.drupal.org documentation for this hook
We ought to prevent invalid theme suggestions from appearing in the twig debug statement
If possible, we should trigger some errors if an invalid theme_suggestion is added

Comment	File	Size	Author
#5	Screenshot 2023-02-21 at 10.44.22 AM.png	163.61 KB	andy-blum

Issue fork drupal-3343198

Show commands

Start within a Git clone of the project using the version control instructions.

Add & fetch this issue fork’s repository

Or, if you do not have SSH keys set up on git.drupalcode.org:

Add & fetch this issue fork’s repository

3343198- compare
Check out this branch for the first time

Check out existing branch, if you already have it locally

1 hidden branch

3343198-improve-documentation-of

changes, plain diff MR !3507

Check out this branch for the first time

Check out existing branch, if you already have it locally

About issue forks

Comments

Comment #1

20 February 2023 at 16:05

andy-blum created an issue. See original summary.

Comment #2

andy-blum

he/him/his

English

Ohio, USA

commented 20 February 2023 at 16:09

Anecdotally, and with zero testing right this second, I seem to recall seeing theme suggestions added with hyphens will appear in the twig debug output as well, but will never be used because they were crafted incorrectly. If that's still the case, can we also make that system able to accept hyphenated suggestions or prevent them from appearing in the twig debug output?

Comment #3

21 February 2023 at 15:39

andy-blum opened merge request !3507

Comment #4

andy-blum

he/him/his

English

Ohio, USA

commented 21 February 2023 at 15:40

Status:

Active

» Needs review

Comment #5

andy-blum

he/him/his

English

Ohio, USA

commented 21 February 2023 at 15:45

Status	File	Size
new	Screenshot 2023-02-21 at 10.44.22 AM.png	163.61 KB

MR updates the theme.api.php doc comments, and filters malformed theme suggestions into a new list where themers can see them.

Comment #6

murilohp commented 21 February 2023 at 17:57

Status:	Needs review	» Needs work
Issue tags:		+Need tests

Hey @andy-blum, the code looks good, just added some stuff there but I'm open to discuss. I think it would be nice to add some tests here, I have to dive into this to see if we have any tests to validate the debug info being rendered, so I'm leaving it as NW and the need tests tag.

Comment #7

andy-blum

he/him/his

English

Ohio, USA

commented 21 February 2023 at 18:26

@murilohp - Thanks for the feedback, I've updated the code with your suggestions. I agree some test coverage would be good, probably as part of TwigDebugMarkupTest.php, though we will likely need to create a test suite module to add in some theme suggestions that will fail.

Comment #8

andy-blum

he/him/his

English

Ohio, USA

commented 21 February 2023 at 18:44

Status:

Needs work

» Needs review

Comment #9

murilohp commented 22 February 2023 at 03:18

Hey, I just added a new hook to the theme_test module with an invalid suggestion and updated the test, let's see how it goes.

Comment #10

DSushmita commented 22 February 2023 at 03:44

The code looks okay to me.

Comment #11

Gazi Akil commented 22 February 2023 at 04:56

This hook allows any module or theme to provide alternative theme function or template name suggestions and reorder or remove suggestions provided by hook_theme_suggestions_HOOK() or by earlier invocations of this hook.

HOOK is the least-specific version of the hook being called. For example, if '#theme' => 'node__article' is called, then node_theme_suggestions_node() will be invoked, not node_theme_suggestions_node__article(). The specific hook called (in this case 'node__article') is available in $variables['theme_hook_original'].

Comment #12

smustgrave commented 22 February 2023 at 19:46

Status:	Needs review	» Needs work
Issue tags:	-Need tests	+Needs Review Queue Initiative

Removing tests tag as I see they were added.

Moving to NW for the failure in MR.

Comment #13

andy-blum

he/him/his

English

Ohio, USA

commented 23 February 2023 at 21:35

Status:

Needs work

» Needs review

Comment #14

24 February 2023 at 12:32

andy-blum credited safetypin.

Comment #15

andy-blum

he/him/his

English

Ohio, USA

commented 24 February 2023 at 12:32

Comment #16

smustgrave commented 24 February 2023 at 15:00

Status:	Needs review	» Needs work
Issue tags:		+Needs change record

Think this will need a change record.

As existing sites may be using these templates, even though they are invalid

Comment #17

andy-blum

he/him/his

English

Ohio, USA

commented 24 February 2023 at 15:22

Status:

Needs work

» Needs review

Change record drafted.

Comment #18

andy-blum

he/him/his

English

Ohio, USA

commented 24 February 2023 at 15:35

Issue tags:

-Needs change record

Comment #19

smustgrave commented 24 February 2023 at 20:27

Status:

Needs review

» Reviewed & tested by the community

Thanks for the quick turnaround on the change record!

Think this is good for next review.

Comment #20

andy-blum

he/him/his

English

Ohio, USA

commented 1 March 2023 at 20:25

Status:

Reviewed & tested by the community

» Needs review

Comment #21

andy-blum

he/him/his

English

Ohio, USA

commented 2 March 2023 at 14:36

Status:

Needs review

» Needs work

Comment #22

andy-blum

he/him/his

English

Ohio, USA

commented 2 March 2023 at 16:02

Status:

Needs work

» Needs review

Comment #23

smustgrave commented 2 March 2023 at 18:07

Status:

Needs review

» Needs work

Additional changes look good to me.
There is 1 thread still open. Should this use the D10 link or D9?

Comment #24

andy-blum

he/him/his

English

Ohio, USA

commented 2 March 2023 at 19:15

Could we craft the link to be whatever version of core is running?

$version = explode('.', \Drupal::VERSION)[0];

$output['debug_info'] .= "\n   See https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Render!theme.api.php/function/hook_theme_suggestions_alter/" . $version;

Would we be able to get the core value in the tests?

Comment #25

andy-blum

he/him/his

English

Ohio, USA

commented 2 March 2023 at 19:31

The other option is to leave the link as-is without a version, and let it resolve to whatever the latest supported version is

Comment #26

smustgrave commented 2 March 2023 at 19:48

Status:

Needs work

» Reviewed & tested by the community

Looking at other examples in core

@link https://api.drupal.org/api/drupal/namespace/Drupal!migrate!Plugin!migrat... List of source plugins provided by the core Migrate module. @endlink

They don't seem to be specifying either so think we can do as #25 mentioned and rely on latest supported version.

Comment #28

andy-blum

he/him/his

English

Ohio, USA

commented 3 March 2023 at 02:10

Removing credit for @Avanishyadav. Simply opening a MR with no commits does not warrant a contrib credit.

@Avanishyadav, if you're genuinely interested in contributing please be sure to adhere to issue ettiquette, and not just gaming the credit system as it appears you're doing in this issue and the issues below:

Empty MR: #2968883: Empty #tag in HtmlTag causes <> to be printed at the top of the pages.
Creating a duplicate issue: #3344672: remove the word "blacklist" from code and documentation
Resubmitting another's solution: #3342535: Create logo for Views Slideshow
Empty MR: #3304184: jQuery UI Slider - Update Categories

Comment #29

volkswagenchick

she/her

English

San Francisco Bay Area

commented 3 March 2023 at 02:30

Thank You @andy-blum for taking a moment to educate @Avanishyadav on issue queue etiquette,

Comment #30

gisle

he/him

Norwegian Bokmål

Norway

commented 3 March 2023 at 05:44

Deleted the bogus MR by Avanishyadav.

Comment #31

3 March 2023 at 08:21

lauriii committed 0ac4b763 on 10.1.x

Issue #3343198 by andy-blum, murilohp, smustgrave, safetypin: Improve...

Comment #32

lauriii

he/him

Finnish

Finland

commented 3 March 2023 at 08:23

Status:

Reviewed & tested by the community

» Fixed

Committed 0ac4b76 and pushed to 10.1.x. Thanks!

I don't think this issue needs CR. It is a DX improvement. I don't think anyone. needs to be actively aware of this feature and I don't see that there would be any action for anyone to take as a result of this.

Should we add similar documentation to hook_theme_suggestions_HOOK docs too?