YamlFileLoaderTest fails if you have PECL yaml installed

Amazingly this also surfaces a bug we made in 2014!

        if ($invalid_keys = array_diff_key($content, array('parameters' => 1, 'services' => 1))) {
            throw new InvalidArgumentException(sprintf('The service file "%s" is not valid: it contains invalid keys %s. Services have to be added under "services" and Parameters under "parameters".', $file, $invalid_keys));
        }

This is causing the array to string conversion in the test... this code has not changed since 2014 in #2269385: DependencyInjection YamlFileLoader is out of date →

So I managed to install pecl yaml in gitlab using the ci file... so https://git.drupalcode.org/project/drupal/-/jobs/357484 shows the failure. I've merged the fixes to MR that does and hopefully we'll see it green.

So yep we can see that the MR fixes this

See https://git.drupalcode.org/project/drupal/-/jobs/357749 - PECL yaml installed + See changes - it is green
See https://git.drupalcode.org/project/drupal/-/jobs/357743 - PECL yaml installed + only test changes - it is broken as expected.

Going to close the MR that installs PECL yaml as that is not the one to commit.

Decided to mark the test only MR as a draft so it's obvious which one to commit but it is also easy to find the testing.

It seems there are two things here:

The first one is this test case

   do not:
      do this for the love of Foo Bar!

will be treated as invalid by YamlSymfony, but valid by YamlPecl

The second is the array to string conversion in exception message for invalid keys. Looking at the expectation message on each test case, it seems invalid keys don't have a test case yet.

In my opinion, we need to make that test case above also invalid if parsed by YamlPecl and make another test case to cover the invalid keys.

I try that this yaml is invalid by YamlPecl

   do not:
      do: this: for the love of Foo Bar!

changing the test case to this make the test passed.

And then after that we can add a test case for invalid keys.

@el7cosmos great ideas... done that and now we have the same coverage on PECL and Symfony. Nice.

One test stull fails

@andypost that's not related to this issue, afaics. \Drupal\Tests\settings_tray\FunctionalJavascript\SettingsTrayBlockFormTest::testBlocks() is failing on HEAD too.

Saw this in #contribute and was curious. I haven't been involved in the YamlPecl stuff to date, so I may be wrong here, but I'm wondering if someone can tell me if I am off here.

From looking at to \Drupal\Component\Serialization\Yaml we always encode via Symphony Yaml, but we decode with YamlPecl if it is available, and fall back to SymphonyYaml if it isn't.

do not:
      do this for the love of Foo Bar!

is valid yaml in yamlPecl but not symfonyYaml.

If we change the test for invalid yaml to something that is invalid on both, and then add PeclYaml to the test containers, does that put us in a position where we could commit some config with the above format, have it pass all tests (as we are using PeclYaml), and then be broken a system that is relying on SymfonyYaml? Do we lint yaml files to the symfony standard somewhere to make sure we are not potentially committing something that Symfony would find invalid? Are there other concerns that if we add YamlPecl to the test containers that we are then not testing anything with the SymfonyYaml fallback? Given than SymfonyYaml and YamlPecl have different opinions on the above format, I wonder what other differences might be lurking that we won't catch later if we take SymfonyYaml out of tests.

Chatted with @alexpott in Slack about this, and I feel better. As mentioned, historically we were testing with YamlPecl, and it is important that we go back to that, to avoid breaking things with the extension, and it seems we have enough compatibility tests to be comfortable with doing so.

I am curious though, (and I haven’t had enough coffee this morning to wade through the spec) whether that format is valid YAML or not according to the YAML spec. if it is, then maybe there should be an issue against symfonyYaml to allow it.

Committed/pushed to 11.x and cherry-picked back to 10.1.x, thanks!

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