(Travis and fabbot failures unrelated)

Couldn't we support this in the YAML Format too by specifying an array with only one element (same for the PHP loader)?

Or null~ first element?

Looks like a good trade-off to me and can be handled directly in theDefinition::setFactory() method.
Anything against supporting both? (i.efactory: [~, 'create'] andfactory: ['create'])

PR updated to support this directly in theDefinition class and allowing it from theYamlFileLoader.

Exceptions messages would need to be enhanced, but it's hard to be really succinct.

Rethinking about this one, shouldn't this logic be handled by PhpDumper instead?
Definition should have as low logic as possible. But PhpDumper is the right place tointerpret what a definitionmeans. Don't you think?

@nicolas-grekas : I can give a look, but the idea was to be able to warn the user with as much context as possible if something is missing, and limit things done in his back (if the definition class is altered somewhere, the end user might not expect the factory class to change magically).

services:my_factory:class:Bar\Bazfactory:[~, 'create']

Is only syntax sugar to avoid typingBar\Baz twice in the definition. Not a magical thing allowing to guess the factory class according to the finalDefinition class if the class has been modified elsewhere, in a compiler pass for instance. Considering this, maybe I shouldn't have changed any logic in theDefinition class, but only added it to dedicated loaders.

Now, if we really want to support something like:

services:my_factory:factory:[~, 'create']

where the class can be provided later, indeedPhpDumper looks like the good candidate.

be able to warn the user with as much context as possible if something is missing

the PhpDumper has full context to do so

I'll rework this PR then.

Status: Needs work

This makes the feature a compilation artifact.. not sure if that makes sense.

Im fine with mutating theDefinition with sensible defaults in runtime as well... perhaps a privateContainerBuilder::finalizeDefinition($id, Def $def) will do?

• set class from id
• set factory from class
• etc.

Updated.

Status: Needs Review

@ogizanagi I know that fabbot.io fail is not related with your changes, but could you fix it in this PR?

@yceruto Those changes should be ignored ;) (seePHP-CS-Fixer/PHP-CS-Fixer#2413).

There is a competition between this PR and#20264: both try to define the class when it's missing based on different rules. But if we're not careful, those rules can collide and we can have priority issues, with one implem taking over the other one.
I think this PR should be enhanced up to replace#20264.

Hum, I completely messed up with my previous comment, you can forget about it :)
Why do we need to patch the xml & yaml loader at all? I'd expect the compiler pass to deal with the logic completely. Did I miss something?

Why do we need to patch the xml & yaml loader at all?

Just in order to be able to use those formats without getting an exception thrown by the loaders because factory class is missing.
The compiler pass does all the job.

(Happy new year 🎉 🎈🍾🥂😃)

The compiler pass is missing recursivity handing into arguments, etc. Note that I'd be really fine with considering that PhpDumper + ContainerBuilder::createService have this responsibility of interpreting such state correctly as I proposed previously...
PhpDumper & ContainerBuilder already have many in common and the logic we're talking about doesn't look that divergent from what they already share.

(Happy new year ! :) )

As I don't have a strong opinion on the Compiler pass vs. PhpDumper + ContainerBuilder, I can update both commits and we can choose then...but that would help me not doing everything twice if someone else has a stronger opinion on this.

Isn't a complete Definition better, for instance when using thedebug:container command?

@ogizanagi can you please amend the first commit so that it gets in what's fixed by the 2nd one (ie everything but the compiler pass).

We miss a test + implementation for ContainerBuilder.

@stof please confirm you'd be ok with only the first commit. Personally, I am. A recursive compiler pass vs 4 lines in the PhpDumper + 4 lines in ContainerBuilder, I prefer the 4x2 lines.

@nicolas-grekas : amended + added implementation for the ContainerBuilder + test.

Despite it's the most straightforward solution, we should be aware that it'll still allow an "incomplete" Definition. Thus, the following:

foo:class:Foofactory:[~, create]

along with thebin/console debug:container foo command, for instance, will output:

Information for Service "foo"============================= ------------------ --------   Option             Value    ------------------ --------   Service ID         foo       Class              Foo       Tags               -         Public             yes       Synthetic          no        Lazy               no        Shared             yes       Abstract           no        Autowired          no        Autowiring Types   -         Factory Class                Factory Method     create   ------------------ --------

(i.e no factory class, unless we adjust the command or opt for the pass)

EDIT: I've added the required changes in descriptors regarding this in the first commit. Let me know.

@stof : Anything to declare? 😄

Otherwise I'll remove the last commit about the compiler pass implementation.

Thank you.

@ogizanagi would you mind updating the compiler pass ti make it able to handle nested inline definitions?

@nicolas-grekas : I was going to remove the second commit and go forward. Did you change your mind, or should we simply wait for more opinions on this?

BTW, regarding the PHPDumper + ContainerBuilder implementation, now that dumping a non-compiled container is deprecated and#21133 is merged,this test is failing when adding$container->compile(), because:

1. [DI] Optional class for named services #21133 uses the service id if class is not set in the definition, even for a service id likebar. If we want to avoid aclass_exists call to support generated classes, I wonder if we shouldn't at least enforce a leading\ in case someone wants to register a service named after a class in the global namespace?
2. TheCheckDefinitionValidityPass anyway checks a definition has a class, even for definition with a factory. So maybe this test should be marked as legacy, or should be removed along with related code?

@ogizanagi can you rebase and take advantage of AbstractRecursivePass now that#21327 is merged?

Tests are failing, but that's not related AFAIK.

👍

@ogizanagi Can you add a note in the component CHANGELOG?

Sure. Done :)

Thank you@ogizanagi.