Allow yml files to nominate imports

Overall I like the idea but my only concern is we already have a lot of yml files in module directory.
If we are going to support this we should follow some kind of naming convention.

Ok then I am +1 completely.

This definitely improves DX. It is nice and simple solution. Patch is green with tests. I am going to RTBC it because imo it can be added to core as is.

This patch looks good!

But...

Sorry, We should really add unit test coverage in the YamlDiscoveryTest class for this though, as this is a component 'n' all.

Is there a reason we don't just support multiple files with the same suffix? With this you would not have to specify the "imports"?

Symfony already does something like this for its services.yml file, although we don't support it yet because we can't use the same parser they do. We should use the same syntax as that, and then support it for both the services.yml and routing.yml files. Definitely +1 in general.

iirc, Symfony does use 'imports', which this patch already does.

Another thought, if we are adding this to our generic YamlDiscovery component, we should probably d oa deep merge of the values rather than just += ?

Reroll for now. Will action #13.

1. +++ b/core/modules/system/tests/modules/router_test_directory/routes/router_test.foo.routing.yml
   @@ -0,0 +1,6 @@
   diff --git a/core/tests/Drupal/Tests/Component/Discovery/Fixtures/test_import/test_import.test.yml b/core/tests/Drupal/Tests/Component/Discovery/Fixtures/test_import/test_import.test.yml
   
   diff --git a/core/tests/Drupal/Tests/Component/Discovery/Fixtures/test_import/test_import.test.yml b/core/tests/Drupal/Tests/Component/Discovery/Fixtures/test_import/test_import.test.yml
   new file mode 100644
   
   new file mode 100644
   index 0000000..0a9edc4
   
   index 0000000..0a9edc4
   --- /dev/null
   
   --- /dev/null
   +++ b/core/tests/Drupal/Tests/Component/Discovery/Fixtures/test_import/test_import.test.yml
   
   +++ b/core/tests/Drupal/Tests/Component/Discovery/Fixtures/test_import/test_import.test.yml
   +++ b/core/tests/Drupal/Tests/Component/Discovery/Fixtures/test_import/test_import.test.yml
   @@ -0,0 +1,3 @@
   
   @@ -0,0 +1,3 @@
   +name: test
   +imports:
   +  - test_import.yml
   diff --git a/core/tests/Drupal/Tests/Component/Discovery/Fixtures/test_import/test_import.yml b/core/tests/Drupal/Tests/Component/Discovery/Fixtures/test_import/test_import.yml
   
   diff --git a/core/tests/Drupal/Tests/Component/Discovery/Fixtures/test_import/test_import.yml b/core/tests/Drupal/Tests/Component/Discovery/Fixtures/test_import/test_import.yml
   new file mode 100644
   
   new file mode 100644
   index 0000000..bab79b8
   
   index 0000000..bab79b8
   --- /dev/null
   
   --- /dev/null
   +++ b/core/tests/Drupal/Tests/Component/Discovery/Fixtures/test_import/test_import.yml
   
   +++ b/core/tests/Drupal/Tests/Component/Discovery/Fixtures/test_import/test_import.yml
   +++ b/core/tests/Drupal/Tests/Component/Discovery/Fixtures/test_import/test_import.yml
   @@ -0,0 +1,3 @@
   
   @@ -0,0 +1,3 @@
   +# Name should not get overwritten from import.
   +name: test_import
   +additional: imported
   
   +++ b/core/tests/Drupal/Tests/Component/Discovery/YamlDiscoveryTest.php
   @@ -44,4 +47,34 @@ public function testDiscovery() {
   +  public function testImport() {
   

   Maybe you will hate me but have you thought about using vfsStream to provide the files? For an example have a look at #2295469: Add support for static permission definitions with *.permissions.yml maybe

2. +++ b/core/tests/Drupal/Tests/Component/Discovery/YamlDiscoveryTest.php
   @@ -44,4 +47,34 @@ public function testDiscovery() {
   +    $this->assertEquals(count($data), count($directories));
   

   What about using assertCount instead? This makes it a little bit more clear

3. +++ b/core/tests/Drupal/Tests/Component/Discovery/YamlDiscoveryTest.php
   @@ -44,4 +47,34 @@ public function testDiscovery() {
   +
   +  }
   

   pointless empty line.

NW for review in #15.,

Here are those changes to the test, also removed the Fixtures dir, but left that off the interdiff.

So, a couple of things we need to work out now:

- My question above about deep merging of array values. I think we probably do want to support that. Thoughts? And also, as a part of that:
- The current patch will not override values that already exist (+=) is this how we want it to behave?
- chx mentioned to me in IRC a little while ago that he would like to see this functionality work recursively. Thoughts on that also?

Yeah certainly c) should be a follow up.

The other two ones aren't simple. I think there are many usecases to override a complete service, in a local environment or just
in production. This will get even more important if most Settings will be part of the container as well.

@damiankloip
What is your oppinion on the merging bit?

I agree with both #18 and #19. Let's implement those changes.

1. +++ b/core/tests/Drupal/Tests/Component/Discovery/YamlDiscoveryTest.php
   @@ -9,31 +9,60 @@
   +      if (!file_exists($directory)) {
   +        mkdir($directory);
   +      }
   

   This is a virtual file system, why should the directory exist already?

2. +++ b/core/tests/Drupal/Tests/Component/Discovery/YamlDiscoveryTest.php
   @@ -44,4 +73,62 @@ public function testDiscovery() {
   +    $this->assertEquals($item['name'], 'test');
   

   the expected data should be on the left side here.

3. +++ b/core/tests/Drupal/Tests/Component/Discovery/YamlDiscoveryTest.php
   @@ -44,4 +73,62 @@ public function testDiscovery() {
   +    // Assert the count and items in the unkeyed data.
   +    $this->assertCount(2, $item['unkeyed_data']);
   +    $this->assertSame(array('value_2', 'value_1'), $item['unkeyed_data']);
   +    // Assert that value_1 has not been overwritten and value is present in
   +    // keyed data.
   +    $this->assertSame('value_1', $item['keyed_data']['value_1']);
   +    $this->assertSame('value_2', $item['keyed_data']['value_2']);
   

   Great test coverage!

Thanks!

1. This is because we are putting two files into the same directory. It helps to keep an array of directory data in this was so we can pass it directly to YamlDiscovery.
2. Changed
3. Hell yeah.

Nice! Just needs some change record.

Right now, potx relies on file name patterns to detect translatable strings in yaml files. So, please either add a visible note in the change record / docs, that the imported files should have the same naming pattern as the parent yaml file, or if we cannot enforce that, open a followup issue in the potx module to handle "import" keys in yaml files.

This is not at all related to configuration.

This looks like really useful functionality! However, we're not quite ready to go here yet because:

1) We still need a change record
2) We're missing the follow-up issue indicated by herom
3) We should also file an upstream issue w/ Symfony to request use of their "imports" YAML support without being forced to also use their Config component (Crell says this would allow us to throw out our routing yaml parser and container yaml parser, in addition to this newly-added code.)

Although, I do admit that I like the syntax used here better than:

imports:
    - { resource: "@AcmeHelloBundle/Resources/config/services.yml" }

which is what it seems to be in Symfony, if I'm reading http://symfony.com/doc/current/book/service_container.html#importing-con... properly. Still. It's a good "open source citizen" thing to do.

Unless I'm mistaken, no upstream library performs a deep-merge of imports. Instead, I think we need/want to merge on a selected depth only.

Closely related, order matters for any merge. However, the term "imports" encompasses a concept of base definitions, and I assume the term was deliberately chosen. Imports are loaded and merged first, in the specified order. Custom/local data is merged into the cumulative imported data.

In other words:

$result = ['services' => []];
$result['services'] += $local['services'];
foreach (array_reverse($imports) as $import) {
  $result['services'] += $import['services'];
}

3) We should also file an upstream issue w/ Symfony to request use of their "imports" YAML support without being forced to also use their Config component (Crell says this would allow us to throw out our routing yaml parser and container yaml parser, in addition to this newly-added code.)

Well, good luck with filing one, but technically their Config component exactly has the abstractions they need for their usecases. Well here is one https://github.com/symfony/symfony/issues/11751 but I don't see any really proper usecase.

Although, I do admit that I like the syntax used here better than:

I am not sure whether you will read this, but this is actual consistent behaviour inside symfony and TWIG itself, see http://twig.sensiolabs.org/doc/api.html#built-in-loaders
We also have support for that inside Drupal itself, so you can use @module inside your twig template. I think this itself is fine, using relative paths is just a different way.

@sun
Well, we want to be able to override existing services/yml files, otherwise you would need to write compiler passes for relative simple things.

we want to be able to override existing services/yml files, otherwise you would need to write compiler passes for relative simple things.

That seems to diverge from the original goal in the issue summary, which is seemingly limited to splitting up YAML files into multiple, for sanity/organization purposes?

I don't think the topic of overrides should be mixed into imports. Override logic typically requires much more granular considerations; e.g., see service provider YAML file processing in DrupalKernel. IMO, imports should be local to a file and should only be used to construct your own definitions. You may only override (replace) definitions that you imported (into your local scope).

The result of the upstream issue was that it does not make sense to be implement in the YAML component, as it is just a parser/writer.
On top of that, the logic for importing other definitions etc. is already handled by the config component, we could indeed use, as it is other kind
of config as we do have. Their config component handles every kind of config in code: routing.yml files, .services.yml files etc.

@larowlan
It is still not clear how we will be able to support both imports and merges ... given that we kind of need both.

Note: http://symfony.com/doc/current/components/config/index.html would be a tool we could use, but I think you will be killed if you suggest it :)

Well, sure we could do that, but this would really just matter for routing.yml files and not longer for even just services files.

Please review this patch.

This would be very valuable for splitting up core.services.yml too.

Here I have the pleasure of quoting @sun:

That seems to diverge from the original goal in the issue summary, which is seemingly limited to splitting up YAML files into multiple, for sanity/organization purposes?

I agree that this most likely makes sense for 8.1, but if it's just reorganization/splitting up then it doesn't actually break BC in any way, it only improves DX. AFAICT at least. So, moving back to 8.0 just in case it's a very small amount of work for an excellent DX improvement.

I guess no one did it...

This probably doesn't need to be postponed anymore.

I was really surprised this is currently not possible so +1 For improving DX.

The question of merge and override still open but there's initial patch, so valid status

Hi all!

Rerolled patch on 9.3.x
Also added same functional for THEME.libraries.yml because i need it for components libraries.

Tests still need reroll

This would be a hugely impactful feature. But would absolutely not be possible to land without tests 🤓

Added a patch for Drupal 10.1.x, still needs work for test.