Create TemplateDiscovery for plugin managers to use

#3064854: Allow Twig templates to use front matter for metadata support was committed.

This should be an easy patch to make. We have the class and tests already in the experimental Help Topics module in core.

See
core/modules/help_topics/src/HelpTopicDiscovery.php (note that it uses the Help Topics FrontMatter class and not the one that is now in core/lib/Drupal/Component/FrontMatter/FrontMatter.php , so that will need an update)
and
core/modules/help_topics/tests/src/unit/HelpTopicDiscoveryTest.php

So basically, the names of these classes need to be changed, and they need to be moved out of the help_topics module into the core/lib namespace, to the same locations as classes/tests for other types of plugin discovery.

At the moment we can't replace core/modules/help_topics/src/HelpTopicDiscovery.php because it has TODO (with outdated link)

        // Only the Help Topics module can provide help for other extensions.
        // @todo https://www.drupal.org/project/drupal/issues/3072312 Remove
        //   help_topics special case once Help Topics is stable and core
        //   modules can provide their own help topics.
        if ($provider !== 'help_topics' && $provider !== $file_name_provider) {
          throw new DiscoveryException("$file file name should begin with '$provider'");
        }

But it's totally fine to move it to component and inherit help_topics from it (to limit discovery by help_topics module)

The only question is namespace for new service (and test)

Good point. So the classes we would create would be *mostly* like the ones currently in Help Topics that I listed in #6. :)

Maybe @markcarver can elaborate because this issue is a blocker for #3075432: Inline .layouts.yml definitions with each template

As I recall core won't accept changes that is not used but we can't use it for help topics as it's not stable yet

This was marked as un-postponed by a Drupal Core maintainer, so I think it is OK to proceed.

I started looking at this, but I realized quickly that we have additional help-topic-specific code in our Discovery class, besides what is in #7:

a) When we discover a help topic twig template, we assign it a plugin class of HelpTopicTwig.

b) When we parse out the front matter meta-data, we look for 'label', 'top_level', and 'related', and we fail if there is anything else.

So... I'm not sure how useful it will really be to do this. We could make some kind of an abstract class with some kind of a validateAndSaveFrontMatter() abstract method I guess? But it's not as simple as just copying the classes we have.

Could be something like this?

As another note, the other plugin discovery classes are all in Drupal\Core but are small wrappers around Drupal\Component classes. I didn't do that here, but probably we should.

Setting NW as there's great starting patch! Thank you @jhodgdon

1. +++ b/core/lib/Drupal/Core/Plugin/Discovery/TemplateDiscovery.php
   @@ -0,0 +1,185 @@
   +   * @param string $file_cache_key_suffix
   ...
   +   * @param string $key
   ...
   +  public function __construct(array $directories, $file_cache_key_suffix, $key = 'id') {
   

   it could use PHP's type hints now to string for last 2 arguments

2. +++ b/core/lib/Drupal/Core/Plugin/Discovery/TemplateDiscovery.php
   @@ -0,0 +1,185 @@
   +   * @return array
   +   *   An array of discovered data keyed by provider.
   ...
   +        $data = [
   +          // The plugin ID is derived from the filename. The extension
   +          // '.html.twig' is removed.
   +          'id' => $plugin_id,
   +          'provider' => $file_name_provider,
   +          static::FILE_KEY => $file,
   +        ];
   ...
   +        $data = $this->validateAndSave($front_matter, $data);
   +        $all[$provider][$data['id']] = $data;
   ...
   +    return $all;
   

   expected keys could use docs

3. +++ b/core/lib/Drupal/Core/Plugin/Discovery/TemplateDiscovery.php
   @@ -0,0 +1,185 @@
   +          /** @var \SplFileInfo $fileInfo */
   +          $iterator = new RegexDirectoryIterator($directory, '/\.html\.twig$/i');
   +          foreach ($iterator as $fileInfo) {
   +            $file_list[$fileInfo->getPathname()] = $provider;
   

   each provider could have own pattern to filter files

I'm working on this. I plan to make a patch that uses it to replace the custom discovery in Help Topics as a proof of concept, as well as cleaning this up.

Here's (I think) a viable patch.

I've attached:
- An interdiff for the generic discovery class as compared to the non-working earlier patch.
- A patch containing just the generic discovery class. This one doesn't really do anything, just adds the class file. Question: should this have both a Core and Component class, as there are in the other Discovery classes? I'm not really sure. Also it probably needs some kind of a test.
- A patch containing that class and the Help Topics module using it. The help topics portion should go onto #3176735: Replace Drupal\help_topics\HelpTopicDiscovery with core/lib Twig discovery class if this issue gets done.

I didn't run all of the help topic classes, but the HelpTopicTest function test does pass locally. Let's see what happens.

Quickly skimmed, looks great and explains the idea!

Code sniff found several unused use statements. Removing from both patches. Interdiff is from the larger "with usage" patch to the new "with usage" patch.

One small adjustment to a test is needed in the "with usage" patch, because the exception message is more generic in this patch.

So here's a new "with usage" patch. No changes to the patch in #17 without usage.

Here's a new patch. I've added some additional docs to the class on how to use it. I did not update the "usage" patch, as this only added docs. This patch still needs tests. Also updated the issue summary on what needs to be done.

Looks ready to be used for templates and tests

Yes, I think it's ready. I plan to make tests for the patch later today (adapt the tests we wrote for the Help Topics specific template discovery).

And based on the "with usage" patch, if this is committed we are ready to use it as-is in Help Topics. The latest "with usage" patch has the exact same class (minus the new docs added in comment #23, which are based on how we can use it in Help Topics, slightly simplified).

And here is the same patch with a test class added. I made no changes to the TemplateDiscovery class, so the interdiff is the added test class.

Something weird is happening on my local system lately when I run unit tests (they hang), so I am not certain whether the test passes or fails. I think it might fail but it should be close.

I have a patch that fixes the one-line code sniff problem but I cannot seem to upload it (getting 5xx server errors).

Here's it

Here's a fix for the test, if I can upload a patch now...

And one more test fix.

There we go -- green! I think this patch is ready to go. @andypost, I think you're eligible to review it if you want, since your making-patch contributions so far were one blank line added. :)

I find it mostly ready, great job!

Could be interesting to try use it for #3075432: Inline .layouts.yml definitions with each template

1. +++ b/core/lib/Drupal/Core/Plugin/Discovery/TemplateDiscovery.php
   @@ -0,0 +1,280 @@
   +  protected $masterProvider;
   ...
   +   * @param string $master_provider
   +   *   (optional) The name of a provider that is allowed to provide plugins on
   +   *   behalf of other extensions.
   ...
   +  public function __construct(array $directories, string $file_cache_key_suffix, string $master_provider = '', string $id_key = 'id', string $file_filter = '/\.html\.twig$/i') {
   ...
   +    $this->masterProvider = $master_provider;
   ...
   +          (empty($this->masterProvider) || $provider != $this->masterProvider)) {
   

   I bet $rootProvider is better wording

2. +++ b/core/lib/Drupal/Core/Plugin/Discovery/TemplateDiscovery.php
   @@ -0,0 +1,280 @@
   +  public function findAll() {
   ...
   +  protected function findFiles() {
   

   Both needs to add `: array` to point that functions return array

3. +++ b/core/lib/Drupal/Core/Plugin/Discovery/TemplateDiscovery.php
   @@ -0,0 +1,280 @@
   +    foreach ($this->directories as $provider => $directories) {
   +      $directories = (array) $directories;
   +      foreach ($directories as $directory) {
   +        if (is_dir($directory)) {
   

   Not clear why cast to array applied

4. +++ b/core/lib/Drupal/Core/Plugin/Discovery/TemplateDiscovery.php
   @@ -0,0 +1,280 @@
   +          /** @var \SplFileInfo $fileInfo */
   +          $iterator = new RegexDirectoryIterator($directory, $this->fileFilter);
   +          foreach ($iterator as $fileInfo) {
   

   The comment could be moved one line down

Great points, thanks!

On item #3, the constructor is documented thus:

   * @param array $directories
   *   Array whose keys are provider (extension) short names, and whose values
   *   can be either a single string path to scan for plugins, or an array of
   *   paths for this provider.

(and similar for $this->directories). So, you don't know if it's a string or an array, and you need to cast to array.

Here's a patch/interdiff fixing the other items from #35.

Thank you, about #3 is clear

Small summary update to say related issue patch has been created/moved. Also created a change notice.
https://www.drupal.org/node/3212845

Test bot flux

1) Drupal\BuildTests\Framework\Tests\BuildTestTest::testPortMany
RuntimeException: Unable to start the web server.

Looks like a random fail in Drupal\Tests\layout_builder\FunctionalJavascript\LayoutBuilderDisableInteractionsTest

Assigning myself to review

1. +++ b/core/lib/Drupal/Core/Plugin/Discovery/TemplateDiscovery.php
   @@ -0,0 +1,280 @@
   + * Sample code for the validateAndSaveFrontMatter() method:
   + * @code
   + * $data['class'] = MyPluginClass::class;
   + * foreach ($front_matter as $key => $value) {
   + *   case 'label':
   + *     $data[$key] = new TranslatableMarkup($value);
   + *     break;
   + *   default:
   + *     throw new DiscoveryException("$file contains invalid key='$key'");
   + * }
   + * if (!isset($data['label'])) {
   + *   throw new DiscoveryException("$file has no label attribute");
   + * }
   + * return $data;
   + * @endcode
   

   As this is being proposed to replace or augment YAML discovery (at least for layouts), it'd be good to have an equivalent method to \Drupal\Core\Plugin\Discovery\YamlDiscovery::addTranslatableProperty(), which LayoutPluginManager calls 3x for label, description, and category.

2. +++ b/core/lib/Drupal/Core/Plugin/Discovery/TemplateDiscovery.php
   @@ -0,0 +1,280 @@
   + *  $discovery = new MyPluginDiscovery(directories, 'my_plugin_cache_key');
   + *  $discovery = new YamlDiscoveryDecorator($discovery, 'my_derivative_suffix',
   + *    $directories);
   

   This example is confusing because it doesn't mention TemplateDiscovery at all (also the code wrapping is needlessly confusing).

   I think this patch should include a TemplateDiscoveryDecorator to make adding it to existing plugins easier. See YamlDiscoveryDecorator for an example.

3. +++ b/core/lib/Drupal/Core/Plugin/Discovery/TemplateDiscovery.php
   @@ -0,0 +1,280 @@
   +abstract class TemplateDiscovery implements DiscoveryInterface {
   ...
   +  abstract protected function validateAndSaveFrontMatter(array $front_matter, array $data, string $file);
   

   Oh now the first comment makes a little more sense. It's not 100% clear to me why it's done this way, requiring an entire new class, instead of letting the plugin manager's processDefinition() handle it (and handling the translatable needs as mentioned above)

4. +++ b/core/lib/Drupal/Core/Plugin/Discovery/TemplateDiscovery.php
   @@ -0,0 +1,280 @@
   +  public function __construct(array $directories, string $file_cache_key_suffix, string $root_provider = '', string $id_key = 'id', string $file_filter = '/\.html\.twig$/i') {
   

   It's not 100% clear why this is restricted to Twig. (I don't know what else we would use it for, just curious)

5. +++ b/core/lib/Drupal/Core/Plugin/Discovery/TemplateDiscovery.php
   @@ -0,0 +1,280 @@
   +  public function findAll(): array {
   

   This method is not backed by any interface and is not called anywhere else, it should be protected/private

6. +++ b/core/lib/Drupal/Core/Plugin/Discovery/TemplateDiscovery.php
   @@ -0,0 +1,280 @@
   +        list($file_name_provider,) = explode('.', $plugin_id, 2);
   

   Nit: I don't think the trailing comma is needed.

7. +++ b/core/lib/Drupal/Core/Plugin/Discovery/TemplateDiscovery.php
   @@ -0,0 +1,280 @@
   +        if ($provider !== $file_name_provider &&
   +          (empty($this->rootProvider) || $provider != $this->rootProvider)) {
   

   This is a rather complex conditional. Also it uses !== in one place and != in another.

   It may seem like overkill, but can the major parts of the conditional be first assigned to helpfully named variables?

8. +++ b/core/lib/Drupal/Core/Plugin/Discovery/TemplateDiscovery.php
   @@ -0,0 +1,280 @@
   +        $data = $this->validateAndSaveFrontMatter($front_matter, $data, $file);
   

   Odd for a "Save" method to return data. Would that be better named Process or Parse?

9. +++ b/core/lib/Drupal/Core/Plugin/Discovery/TemplateDiscovery.php
   @@ -0,0 +1,280 @@
   +   * Your method should throw an exception of there is an error in the front
   

   "throw an exception IF there is an error"
   Also what type should the exception be? We should decide and then this method should have an @throws (potentially instead of this comment)

Noting that there is a contrib module implementation of parts of this feature here: https://drupal.org/project/patternkit

It may have some helpful programming patterns and design decisions for this patch if jhodgdon and tim.plunkett could take a look.

I will have some time in November, but not before then and I don't want to delay this functionality.

Hello all! Created patch based on previous #35 and comments from #44

1. Added test patch where twig template defines own layout and own library. To check results you should apply both patches and enable Layout Builder and Layout Discovery.
2. From my point of view we shouldn't name it TemplateDiscovery since we search in frontmatter here. Frontmatter may also appears in markdown or anything.
   So FrontMatterDiscovery.
3. Search is recursive because it can be useful in case of components libraries hierarchy.
4. I think we need to use some custom file extension to optimize search perfomance so file could be named FILE_NAME.frontmatter.html.twig
5. I think that that frontmatter part could contain some inner data like default variables values
   https://jekyllrb.com/docs/front-matter/#custom-variables
   what can be very useful in components driven development with storybook or similar.
   So plugins should be defined in `plugins` section of yaml frontmatter.
   But this is optional in this patch and can be controlled with $array_position variable
6. Tests and usage examples documentation should be added later based on example patch.

Tried to fix the custom command failure errors in both the patches provided in #45 and attaching an interdiff as well for reference.

Thanks!

The last patches failed with "PHP 8.0 & MySQL 5.7 Custom Commands Failed" but the status wasn't changed automatically to "Needs work", so changing status now. Also, these patches should now be tested/updated/reviewed on 9.4.x. Thank you.

fix cspell and combined example patch

fix phpstan and needs to add tests from #35

This one has gone cold. Are people still interested in getting this in?

It needs subsystem manager review as it used by help module only

@andypost -- Are you looking for a high-level overview of the approach in #53 from a Theme API subsystem maintainer before continuing work on it? Do you want me to ping maintainers in Slack to take a look, or do you want to make any updates to the patch first?