Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[DependencyInjection] ActionBundle integration: introduce _instanceof#21357
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
you can still have something like:
$parentId =$baseParentId =md5("$type.$id");$i =0;while ($this->container->has($parentId)) { ++$i;$parentId =$baseParentId.$i;}
dunglas commentedJan 24, 2017
Status: needs review |
| } | ||
| if ($parentId) { | ||
| $s->setAttribute('parent',$parentId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Fixed.
| return$definition; | ||
| } | ||
| $className =$definition->getClass() ?:$id; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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' =>newDefinition(/*...*/),'type2' =>newDefinition(/*...*/)]]]
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍 to deprecate all services starting by_.
nicolas-grekasJan 24, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm mixed on this deprecation at the general service id level.
But I'm 👍 for deprecating such idswhen 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Btw, we have an inconsistency here:_defaults and_instanceof are allowed as service ids in other loaders.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Will be fixed if we deprecate all services starting with a_ as you suggested.
stof commentedJan 24, 2017
status: needs work |
| */ | ||
| protectedfunctiongenerateInstanceofDefinitionId($id,$type,$file) | ||
| { | ||
| returnsprintf('%s_%s_%s',$id,$type,hash('sha256',$file)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Likesprintf('0%s_%s_%s', $id, $type, hash('sha256', $file));?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
done
nicolas-grekas commentedFeb 4, 2017
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.ymlservices: _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" definitions2fb6019 [DependencyInjection] Add "instanceof" section for local interface-defined configs
Uh oh!
There was an error while loading.Please reload this page.
There is some work being done to include features ofDunglasActionBundle 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.