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

Given we already support the 'service_collector' tag to avoid needing a compiler pass (which I think may have inspired the symfony feature) - do we need this?

Sample from core:

the collector:

cache_tags.invalidator:
    parent: container.trait
    class: Drupal\Core\Cache\CacheTagsInvalidator
    calls:
      - [setContainer, ['@service_container']]
    tags:
      - { name: service_collector, call: addInvalidator, tag: cache_tags_invalidator }

and a tagged service:

cache_tags.invalidator.checksum:
    class: Drupal\Core\Cache\DatabaseCacheTagsChecksum
    arguments: ['@database']
    tags:
      - { name: cache_tags_invalidator}

In this scenario, addInvalidator is called on Drupal\Core\Cache\CacheTagsInvalidator for every service tagged with cache_tags_invalidator

See \Drupal\Core\DependencyInjection\Compiler\TaggedHandlersPass

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 #2961380: Support IteratorArgument in \Drupal\Component\DependencyInjection\Dumper\OptimizedPhpArrayDumper::dumpValue 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 those lines, we could also use !service_closure to define services following this: #3108020: Support ServiceClosureArgument in \Drupal\Component\DependencyInjection\Dumper\OptimizedPhpArrayDumper::dumpValue. Though I have found that autowiring closures (using annotations) solves that since the arguments don't even need to be defined in YAML.

Created a MR from the patch in #8

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 class 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 #3414647: Always use YamlSymfony for loading service yaml files

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 #3205480: Drop PECL YAML library support in favor of only Symfony YAML - this intentionally makes the pecl and symfony parsers different and I don't think we should do that.

Let's use this status, then.

#3205480: Drop PECL YAML library support in favor of only Symfony YAML landed!

rebased

This is just adding the flag and some tests, the dependent issue landed, back to RTBC as per #33.

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

Unfortunately, this commit came after #3205480: Drop PECL YAML library support in favor of only Symfony YAML 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.

#3414208: Add support for tagged_iterator to YamlFileLoader 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...

Added a new child issue #3450516: Add support for !service_closure custom tag in YamlFileLoader