We currently use yaml discovery for both routing and info files, yet they do not share code past the symfony yaml parser. This makes it more difficult for other subsystems that want to use yaml discovery as well as coupling the code that handles routing and info files to yaml unnecessarily.

The need for a generic component has come up in other issues (#2005716: Promote EntityType to a domain object, #2050227: Add local action plugin deriver to use YAML discovery for static definitions), so I think it's time we added this.

The attached patch is extracted from what I wrote for #2005716: Promote EntityType to a domain object.

Here's an example of how it'd be used:


// Finds and parses all modulename.somename.yml files
$directories = $this->moduleHandler->getModuleDirectories();
$discovery = new YamlDiscovery("somename", $this->directories());
$discoverable_things = $discovery->findAll();
CommentFileSizeAuthor
#15 2050289-15.patch13.44 KBdamiankloip
#15 interdiff-2050289-15.txt1.06 KBdamiankloip
#13 2050289-13.patch13.53 KBdamiankloip
#11 2050289-11.patch9.77 KBdamiankloip
#8 2050289-8.patch8.97 KBdamiankloip
#8 interdiff-2050289-8.txt5.92 KBdamiankloip
#3 2050289-3.patch6.32 KBdamiankloip
yaml_discovery.patch3.41 KBmsonnabaum
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

fubhy’s picture

fubhy CreditAttribution: fubhy commented
Status: Needs review » Needs work

Let's move this to Drupal\Components, add some docblocks, and write a UnitTest!

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented

on it.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Status: Needs work » Needs review
FileSize
2050289-3.patch6.32 KB

Ok new patch:

- Moved the class to Component
- Added a fileBaseName method so it can easily be overridden if the pattern of dir_basename.something.yml needs to be changed
- Added a unit test with some test yml files in a Fixtures (change the name? based it on Symfony test files) folder

msonnabaum’s picture

msonnabaum CreditAttribution: msonnabaum commented

Looks great to me.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

It would be cool if there would be a single place where this functionality is actually used.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Assigned: Unassigned » damiankloip

I'll maybe convert RouteBuilder to use this, as the other issues in the summary aren't in yet

msonnabaum’s picture

msonnabaum CreditAttribution: msonnabaum commented

DrupalKernel would be the other option for implementation.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
FileSize
interdiff-2050289-8.txt5.92 KB
2050289-8.patch8.97 KB

I decided to just go with the Routing example, I already had that one in my head :)

I have changed the YamlDiscovery slightly, as the return from findAll() needs to return its discovered data my the module key that was passed in with the directories array, so essentially the directory path gets replaced by an array of discovered data.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented

Oh, maybe I should add back in the !empty() check on routes? or, we could add checking in the YamlDisocvery class to default to array() is nothing is returned from a file?

Status: Needs review » Needs work

The last submitted patch, 2050289-8.patch, failed testing.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Assigned: damiankloip » Unassigned
Status: Needs work » Needs review
FileSize
2050289-11.patch9.77 KB

Looks like layout_test module is the only module in core to not be inside it's own folder. Moved from modules/layout/tests/layout_test.* to modules/layout/tests/layout_test/layout_test.*

Status: Needs review » Needs work

The last submitted patch, 2050289-11.patch, failed testing.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Status: Needs work » Needs review
FileSize
2050289-13.patch13.53 KB

Silly, I forgot to move all the other sub dirs for layout_test.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented 
+++ b/core/lib/Drupal/Core/Routing/RouteBuilder.phpundefined
@@ -81,25 +82,19 @@ public function rebuild() {
+        $defaults = isset($route_info['defaults']) ? $route_info['defaults'] : array();
+        $requirements = isset($route_info['requirements']) ? $route_info['requirements'] : array();
+        $options = isset($route_info['options']) ? $route_info['options'] : array();
+        $route = new Route($route_info['pattern'], $defaults, $requirements, $options);

Can't we just do $route_info += array(
'defaults' => array(),
'requirements' => array(),
'options' => array()
);

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Category: feature » task
FileSize
interdiff-2050289-15.txt1.06 KB
2050289-15.patch13.44 KB

Sure, why not. Then we just need to pass each array element into Route() params.

Also this just seems like more of a refactoring task than a feature request?

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Reviewed & tested by the community

Thank you!

Crell’s picture

Crell CreditAttribution: Crell commented

As noted in IRC, a side-effect of this change is that the routing alter event is only called for modules that define routes, not for all modules. I consider that a bonus as it's a slight performance boost. +1 from me.

alexpott’s picture

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

Committed 03e535c and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

This introduced a regression, see #2067809: YamlDiscovery uses basename of directory instead of module name