Support IteratorArgument in \Drupal\Component\DependencyInjection\Dumper\OptimizedPhpArrayDumper::dumpValue

I only found one nit to pick on this.

+++ b/core/tests/Drupal/Tests/Component/DependencyInjection/Dumper/OptimizedPhpArrayDumperTest.php
@@ -336,6 +337,13 @@ public function getDefinitionsDataProvider() {
+          'arguments' => [new IteratorArgument([new Reference('bar')])],
+          'arguments_count' => 1,
+          'arguments_expected' => $this->getCollection([$this->getCollection([$this->getServiceCall('bar')])]),
+        ] + $base_service_definition;

The indentation here looks off.

Do you mean the + being on the same line as the closing array bracket?

+        ] + $base_service_definition;

I'm sure I've seen that before in core for that pattern of adding defaults to an array.

No, I mean that the items in the array are intented +4 spaces instead of +2.

Oh that’s definitely wrong.

Sorry, was doing patch reviews while the baby was keeping me up at night!

Adding support for ServiceClosureArgument too.

Hi @alexpott thanks for the advice. l have created a new issue concerning ServiceClosureArgument.
l will also add a failing test in the patch that goes with that issue.

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.

patch in #16 needs a reroll.

Rerolled #16 for 10.1.x

Looks solid, we have test coverage, so now need a change record.

Tried to fix #26.

I did a reroll of the patch for 11.x. It should be in the merge request.

hide patch files, using merge request

Added a change record. Reroll looks good. Same restrictions apply as in #3108020: Support ServiceClosureArgument in \Drupal\Component\DependencyInjection\Dumper\OptimizedPhpArrayDumper::dumpValue:

This feature is not supported in the custom YamlFileLoader Drupal is currently using. But for service definitions built programmatically, this patch still brings some benefits.

(also added a change record)

The diff no longer applies. Can someone update the MR?

I read the issue summary and comments. All questions have been answered. The only thing I don't see is a failing test.

Ran the tests without the fix and got

Symfony\Component\DependencyInjection\Exception\RuntimeException : Unable to dump a service container if a parameter is an object without _serviceId.

Checking back in. I read the IS and the comments since my last comment. I didn't find any unanswered questions.

This time I read the change record. The CR title should be a clue to the reader of what the change is. This title should be more specific, perhaps state what now supports and IteratorArgument. The body itself should, ideally, begin with the benefit of this change and when the benefits will be seen. Because of this I am tagging for CR updates and setting back to needs work. Fortunately, that shouldn't take much time for anyone familiar with this change.

Leaving at RTBC.

Updated the CR title and description to make it clearer what the benefits of this change are.

Might need reviews of that CR especially whether it allows the !tagged_iterator argument type now 🤔

Adding #3228629: [meta] Bring dependency injection more in line with upstream Symfony component as parent.

Committed 8b6814e and pushed to 11.x. Thanks!

Updated the change record to reference 10.3.0 and published it.

I removed the services.yml example from the CR. The !tagged_iterator notation still doesn't work. This issue here only changed the container array dumper/loader and not the yaml part (see also #38).

Attempting to use the !tagged_iterator notation results in the following exception:

Symfony\Component\DependencyInjection\Exception\InvalidArgumentException: The file "core/modules/mailer/mailer.services.yml" does not contain valid YAML: Tags support is not enabled. Enable the "Yaml::PARSE_CUSTOM_TAGS" flag to use "!tagged_iterator" at line 42 (near "arguments: [!tagged_iterator mailer.transport_factory]"). in Drupal\Core\DependencyInjection\YamlFileLoader->loadFile() (line 426 of /home/lo/src/drupal/core/lib/Drupal/Core/DependencyInjection/YamlFileLoader.php).

Drupal\Core\DependencyInjection\YamlFileLoader->loadFile('core/modules/mailer/mailer.services.yml') (Line: 71)
Drupal\Core\DependencyInjection\YamlFileLoader->load('core/modules/mailer/mailer.services.yml') (Line: 1303)
Drupal\Core\DrupalKernel->compileContainer() (Line: 941)
Drupal\Core\DrupalKernel->initializeContainer() (Line: 496)
Drupal\Core\DrupalKernel->boot() (Line: 709)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)
require('/home/lo/src/drupal/index.php') (Line: 48)

#50 I believe we need #3108309: Support Yaml::PARSE_CUSTOM_TAGS in \Drupal\Component\Serialization\YamlSymfony::decode for the Yaml loader to support it.

We need to update our own YamlParser to support it.

Added #3414208: Add support for tagged_iterator to YamlFileLoader

Reroll for 10.5.x for those interested. I'm sorry I uploaded the wrong patch.

Here is reroll for 10.5.x