Support Yaml::PARSE_CUSTOM_TAGS in \Drupal\Component\Serialization\YamlSymfony::decode

The Needs Review Queue Bot → tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Following 📌 Support IteratorArgument in \Drupal\Component\DependencyInjection\Dumper\OptimizedPhpArrayDumper::dumpValue RTBC if we implemented this then we could also use !tagged_iterator and friends from Symfony.

if we implemented this then we could also use !tagged_iterator and friends from Symfony.

Along this those lines, we could also use !service_closure to define services following this: [#3360604]. Though I have found that autowiring closures (using annotations) solves that since the arguments don't even need to be defined in YAML.

kim.pepper → made their first commit to this issue’s fork.

Created a MR from the patch in #8

kim.pepper → changed the visibility of the branch 3108309-support-yamlparsecustomtags-in to hidden.

Fixing title.

Given we added constant parsing as well, can we add a test for that too?

What do we need to add to the PECL parser to support this?

Also, is there any additional overhead in adding this parsing? Is there a case where we might not want this flag to be enabled?

@longwave I don't really think there is much overhead - given we would already be parsing the tag in Symfony and throwing an error. The question is really can we duplicate this functionality in PECL. And the problem is we cannot. Looking at the underlying PECL code (https://github.com/php/pecl-file_formats-yaml/blob/php7/parse.c#L641) there is no way to subscribe a callback to all tags. We would need to file an issue and get it into PECL yaml ... not fun. Maybe we could make it make it really simple to register new tags to a PECL yaml callback that produces TaggedValue objects.

@alexpott should we just force the Symfony parser in the DI YamlFileLoader, as that's currently the only place (I think) that we need this?

Further to #24 we already have #2951046: Allow parsing and writing PHP constants and enums in YAML files → to enable PARSE_CONSTANT which also works on PECL and has tests, so let's only work on custom tags here.

Re #27 I was tempted to suggest that. I was reticent because service.yml files are one of the thing parsed on cold cache. But the more I think about it the more I think that this is a good approach for now. The capabilities of the YAML parser and container configuration are quite closely related so this does not feel like a bad decision - also we use FileCache here so potentially this has no impact on cold cache as that can persist across a cache rebuild.

Created 📌 Always use YamlSymfony for loading service yaml files Needs review

Does what it says, has test coverage. Not even sure we should bother with catching the exception in the test, just let it fail if the exception is thrown, but the code is already written so let's leave it as-is.

I don't think we should do this until we've removed support for PECL yaml in 📌 PECL YAML library supports YAML 1.1, but Symfony supports YAML 1.2 Active - this intentionally makes the pecl and symfony parsers different and I don't think we should do that.

Let's use this status, then.

📌 PECL YAML library supports YAML 1.1, but Symfony supports YAML 1.2 Active landed!

rebased

This is just adding the flag and some tests, looks good to me.

Committed and pushed a91f4f4e5b to 11.x and c6a3077802 to 10.3.x. Thanks!

Unfortunately, this commit came after 📌 PECL YAML library supports YAML 1.1, but Symfony supports YAML 1.2 Active which means we added the flag to the deprecated \Drupal\Component\Serialization\YamlSymfony::decode() method.

We need to make the change to \Drupal\Component\Serialization\Yaml::decode() now.

📌 Add support for tagged_iterator to YamlFileLoader Active is doing that already, so I don't know if we need a separate dedicated issue to do that.

@kim.pepper whoops nice spot and yeah sure let's do it in that issue.

Looks like @kim.pepper did that as of ~14 hours ago: https://git.drupalcode.org/project/drupal/-/merge_requests/6134/diffs?co...

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

kim.pepper → closed merge request !6133

Added a new child issue 📌 Add support for !service_closure custom tag in YamlFileLoader Fixed