[DependencyInjection] ActionBundle integration: introduce _instanceof #21357
Conversation
| $s->setAttribute('parent', $parentId); | ||
| } | ||
| // TODO: move the ID generation or maybe the whole block in the parent class | ||
| $parentId = md5("$file.$type.$id"); |
There was a problem hiding this comment.
Including the full file name is a bad idea... It makes the md5 related to the absolute path of the file. It will create deployment problems and it's why test fails. I'll change that.
There was a problem hiding this comment.
you can still have something like:
$parentId = $baseParentId = md5("$type.$id");
$i = 0;
while ($this->container->has($parentId)) {
++$i;
$parentId = $baseParentId.$i;
}| } | ||
|
|
||
| if ($parentId) { | ||
| $s->setAttribute('parent', $parentId); |
There was a problem hiding this comment.
shouldn't it throw an exception if there is already a parent?
| $s->setAttribute('parent', $parentId); | ||
| } | ||
| // TODO: move the ID generation or maybe the whole block in the parent class | ||
| $parentId = md5("$file.$type.$id"); |
There was a problem hiding this comment.
you can still have something like:
$parentId = $baseParentId = md5("$type.$id");
$i = 0;
while ($this->container->has($parentId)) {
++$i;
$parentId = $baseParentId.$i;
}|
Status: needs review |
| } | ||
|
|
||
| if ($parentId) { | ||
| $s->setAttribute('parent', $parentId); |
There was a problem hiding this comment.
There is an issue here. This is altering the DOM node, which means it will impact the next services too (which will break things if they are not ChildDefinition themselves)
| return $definition; | ||
| } | ||
|
|
||
| $className = $definition->getClass() ?: $id; |
There was a problem hiding this comment.
this is broken when the class is a parameter (IIRC, this is still supported in 3.x, even though Symfony itself does not use the feature anymore)
There was a problem hiding this comment.
IIUC, it cannot be fixed in the loader directly because all parameters may have not been set at this time...
There was a problem hiding this comment.
We have 2 options here:
- Don't support parameters as id when using _instanceof and document this behavior
- Store the something like the following and the container and create child definitions in a compiler pass
['/foo/bar.yaml' => [
'services' => ['id1', 'id2'],
'instanceof' => ['type1' => new Definition(/*...*/), 'type2' => new Definition(/*...*/)]
]]For the sake of simplicity, I'm for the solution 1. Solution 2 will create new edges cases (when using inline definitions for instance).
Or maybe someone have a better idea?
| } | ||
|
|
||
| if (isset($underscoreParam['alias']) || isset($underscoreParam['class']) || isset($underscoreParam['factory'])) { | ||
| @trigger_error(sprintf('Giving a service the "%s" name is deprecated since Symfony 3.3 and will be forbidden in 4.0. Rename your service.', $name), E_USER_DEPRECATED); |
There was a problem hiding this comment.
should we deprecate using _ as the first char of a service id entirely to open the door for future reserved names ?
All shortcuts we are adding in 3.3 will make it almost impossible to implement such detection in 3.4+ (class is now optional)
There was a problem hiding this comment.
👍 to deprecate all services starting by _.
There was a problem hiding this comment.
I'm mixed on this deprecation at the general service id level.
But I'm 👍 for deprecating such ids when using the YamlFileLoader
| } | ||
|
|
||
| if (isset($underscoreParam['alias']) || isset($underscoreParam['class']) || isset($underscoreParam['factory'])) { | ||
| @trigger_error(sprintf('Giving a service the "%s" name is deprecated since Symfony 3.3 and will be forbidden in 4.0. Rename your service.', $name), E_USER_DEPRECATED); |
There was a problem hiding this comment.
Btw, we have an inconsistency here: _defaults and _instanceof are allowed as service ids in other loaders.
There was a problem hiding this comment.
Will be fixed if we deprecate all services starting with a _ as you suggested.
|
status: needs work |
| */ | ||
| protected function generateInstanceofDefinitionId($id, $type, $file) | ||
| { | ||
| return sprintf('%s_%s_%s', $id, $type, hash('sha256', $file)); |
There was a problem hiding this comment.
generated ids should start with a number so that we don't confuse them with classes (maybe not applicable here, but still a good default practice to me).
There was a problem hiding this comment.
Like sprintf('0%s_%s_%s', $id, $type, hash('sha256', $file));?
|
Continued in #21530 |
…al interface-defined configs (nicolas-grekas, dunglas) This PR was merged into the 3.3-dev branch. Discussion ---------- [DependencyInjection] Add "instanceof" section for local interface-defined configs | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - This is a direction follow up of #21357 on which we're working together with @dunglas. From the description posted there: There is some work being done to include features of [DunglasActionBundle](https://github.com/dunglas/DunglasActionBundle) in the core of Symfony. The goal of all those PRs is to improve the developper experience of the framework, allow to develop faster while preserving all benefits of using Symfony (strictness, modularity, extensibility...) and make it easier to learn for newcomers. This PR implements the tagging feature of ActionBundle in a more generic way. It will help to get rid of `AppBundle` in the the standard edition and to register automatically some classes including commands. Here is an example of config (that can be embedded in the standard edition) to enable those features: ```yaml # config/services.yml services: _defaults: autowire: ['get*', 'set*'] # Enable constructor, getter and setter autowiring for all services defined in this file _instanceof: Symfony\Component\Console\Command: # Add the console.command tag to all services defined in this file having this type tags: ['console.command'] # Set tags but also other settings like "public", "autowire" or "shared" here Twig_ExtensionInterface: tags: ['twig.extension'] Symfony\Component\EventDispatcher\EventSubscriberInterface: tags: ['kernel.event_subscriber'] App\: # Register all classes in the src/Controller directory as services psr4: ../src/{Controller,Command,Twig,EventSubscriber} ``` It's part of our 0 config initiative: controllers and commands will be automatically registered as services and "autowired", allowing the user to create and inject new services without having to write a single line of YAML or XML. When refactoring changes are also automatically updated and don't require to update config files. It's a big win for rapid application development and prototyping. Of course, this is fully compatible with the actual way of defining services and it's possible to switch (or mix) approaches very easily. It's even possible to start prototyping using 0config features then switch to explicit services definitions when the project becomes mature. Commits ------- 773eca7 [DependencyInjection] Tests + refacto for "instanceof" definitions 2fb6019 [DependencyInjection] Add "instanceof" section for local interface-defined configs
There is some work being done to include features of DunglasActionBundle in the core of Symfony. The goal of all those PRs is to improve the developper experience of the framework, allow to develop faster while preserving all benefits of using Symfony (strictness, modularity, extensibility...) and make it easier to learn for newcomers.
This PR implements the tagging feature of ActionBundle in a more generic way. It will help to get rid of
AppBundlein the the standard edition and to register automatically some classes including commands.Here is an example of config (that can be embedded in the standard edition) to enable those features:
It's part of our 0 config initiative: controllers and commands will be automatically registered as services and "autowired", allowing the user to create and inject new services without having to write a single line of YAML or XML.
When refactoring changes are also automatically updated and don't require to update config files. It's a big win for rapid application development and prototyping.
Of course, this is fully compatible with the actual way of defining services and it's possible to switch (or mix) approaches very easily. It's even possible to start prototyping using 0config features then switch to explicit services definitions when the project becomes mature.
Thanks a lot to @nicolas-grekas for defining the
_instanceofsyntax and giving hints for the implementation.