Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork1.1k
Integrate DunglasActionBundle#960
Uh oh!
There was an error while loading.Please reload this page.
Conversation
app/config/config.yml Outdated
| action:[ ] | ||
| console:[ Command ] | ||
| directories:# List of directories relative to the kernel root directory containing classes to auto-register. | ||
| action:[ '../src/AppBundle/Controller' ]# Still an autowiring_type definition to add in Symfony to avoid this |
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 think it would be great to usesymfony/finder in your bundle to be able to use something like../src/*/Controller instead.
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.
Why not.
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.
It should default to something like that, to avoid registering controllers and services from third-party bundles. I'll do it. That way, we can easily mapController andCommand without any risk to interfere with commands and controllers defined by public bundles.
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 don't think it's worth any extra overhead, and there's no precedent for it in other service imports.
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 didn't expect the configuration to work like this. It had never occurred to me to do any sort of autowiring acrossall bundles - I initially don't like that idea, and it complicates the configuration (I had to research what the difference was between theautodiscover anddirectories keys).
Also:
- what causes console commands to autowire? Are the
actionandconsolekeys significant? I think they are. If so, it's weird to me toalso allow other keys - likefoo(used in the bundle README). I understand that this is to allowother directories to be autowired, but I don't like thatsometimes the keys are special, and sometimes not. It also means we can't catch typos. - You mentioned
Still an autowiring_type definition to add in Symfony to avoid this- what
does this refer to? - Would it be possible to avoid the special
action-annotationroute type altogether? In other words, extend@Routeto use a service definition for the controller if one exists? I ask because itfeels like there are two ways to auto-register a controller as a service: either by using some config inconfig.ymlor by using theaction-annotation. The line between what each does is blurry. I think extending@Routemay be a non-trivial task, but it makes a lot more sense to me: it's in the spirit of autowiring where we (in this case the@Routesystem) effectively "asks" the container for the service for an instance and it returns it.
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, I am cautiously interested about all of this - my criticisms are because if it wewere to go to the trouble of supporting this, let's do itreally well.
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.
Yea, I see now thataction-annotation does nothing more than auto-set the_controller to theaction.* pattern. That feels weak.
A more generic solution where@Route justworks with service controllers looks even more appealing. In fact, if we could pull it off, then it could work in YAML too using the existing syntax - e.g.
homepage:path:/defaults:# or using the A:B:C syntax_controllers:AppBundle\Controller\MainController::homepage
If thatclass were represented as a service, it could/would use that service automatically. Otherwise, it would create a new instance like now.
I'm assuming you've thought about this - it would require a service that contained a run-time class => service id map for resolution.
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 agree with most of your arguments and I've made changes accordingly in the bundle:
- The "autodiscover" config option have been removed, only user bundles are now scanned by default, it's safer in better on all front (Support traditional Symfony conventions dunglas/DunglasActionBundle#26)
- Standard
ControllerandCommandare now the default config (Support traditional Symfony conventions dunglas/DunglasActionBundle#26) - it allows to be as close as possible to current Symfony conventions - The
action-annotationrouting type is being removed, the usualannotationwill work (Support standard annotation type in prefix dunglas/DunglasActionBundle#29)
I'm not confortable with adding some magic in the router to guess the service corresponding to the type. The router supports services for a while using the 'service_name:method' syntax for while. It's what use the bundle. I prefer to keep it as is it will be easier to debug.
For the missing autowiring_type, it's forKernelInterface but it's not related anymore to this PR.
| password:"%mailer_password%" | ||
| spool:{ type: memory } | ||
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.
empty lines should be removed
wouterj commentedMar 25, 2016
I'm still not very sure if this is a great move for thestandard edition. It can of course be great to use this bundle while developing applications, but the boilerplate code in the standard edition plays a big role in the learning process of a developer. Automatically injecting the container and having access to a Also, I'm against recommending controllers as service as the best practice. Most of the time, it only makes ones life harder (there are lots of questions about controller as services on IRC and not that much about normal controllers). Anyway, that's just my personal opinion, so feel free to just ignore it :) |
fabpot commentedMar 26, 2016
Just for the record, I'm also very against controllers as services since they were introduced. I still think this is an anti-pattern. |
dunglas commentedMar 26, 2016
@wouterj, as one of the main documenter of Symfony, it's important to convince you of benefits of this approach.
IMO, the "new" controller system is less magical and less Symfony-specific than the current one. And so, it is less difficult to explain it to complete newcomers: Currently:
It's a lot to learn and it's mandatory even to create a small application. Many parts are not obvious when you are a newcomer. It's obvious for us because we use Symfony for years, but when I teach Symfony to students having only a small Java background, I can say that it's hard to dive into the framework. IMO, all those stuffs to learn initially in Symfony partially explain that many developers decide to start with Laravel instead of Symfony (you dive very easily into Laravel - and their marketing is awesome but it's not related). On the other hand, here is how the new system works:
IMO (but I can be wrong), it's far less concept to learn to dive into Symfony, and it's an improved developper experience:
You have a working Symfony application. How can it be easier?
Controller as a service is an implementation detail of my bundle, we don't have to (and we shouldn't) introduce this notion to newcomers (I haven't mentioned it in the first part of my comment). |
dunglas commentedMar 26, 2016
@fabpot I usually never use controller as a service too in private projects. As explained in my previous comment the goal here isn't to promote controller as a service as best practice (it can be a "hidden" detail). It's just a way to enable automatic object construction trough autowiring. |
gnugat commentedMar 26, 2016
@dunglas: I know it might not sound that good, but would you consider creating a "Symfony Action Edition"? It could be promoted in thelist of distributions. |
Pierstoval commentedMar 26, 2016
There's a dedicated discussion about editions in symfony/symfony-installer#39 |
gquemener commentedApr 15, 2016
Just adding my personnal thought, this is the same debat as the one that occurs about the KnpLabs RAD (Rapid Application Development) Bundle a few years back. This bundle intends to add many auto registration features (by following simple convention) among other things and question was raised about integrating it in the Symfony Standard Edition. The point was then that Symfony Standard was not opinonated and thus would never rely on such project. Solution was to createour own edition, which is a pretty acceptable solution IMO. |
Pierstoval commentedApr 15, 2016
@gquemener thanks for pointing the fact that you created a special Symfony edition, because it is another argument in the debate about Symfony editions created in symfony/symfony-installer#39 😉 |
GuilhemN commentedAug 10, 2016
What do you think about instead adding a new config option to the I think this is the best option as it doesn't force us to modify the standard edition temporarily (as DunglasActionBundle would probably have been imported in the core after some time), neither to change conventions, change bests standards and whatever. This would only be a new feature that may be used or not. |
wouterj commentedAug 10, 2016
Why would FrameworkBundle need Dunglas's bundle config, but not that of FOSRestBundle for instance? |
GuilhemN commentedAug 10, 2016
@wouterj I don't think we can make comparisons here as FOSRestBundle is very specific to API management whereas DunglasActionBundle can be used in any project. |
wouterj commentedAug 10, 2016 • 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.
Alright, why would Frameworkbundle need Dunglas's bundle config, but not that ofTacticianBundle? In order words, Dunglas's bundle is nice, but I have no idea why it is more "core", more "nice", more "important to show to Symfonyians" than other bundles around there. Why would Dunglas's bundle need such special treatment? |
GuilhemN commentedAug 10, 2016 • 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.
BTW we're not talking about the same amount of lines. Tactician is composed of many files, here we're talking about maybe two hundred lines without the tests. But I insist on the fact that it wouldn't be a new standard but only a new feature. Edit: ok, i got the principle of tactician. |
bocharsky-bw commentedAug 10, 2016
What's problem simply to add this bundle manually with |
GuilhemN commentedAug 10, 2016
@bocharsky-bw I don't think an entire bundle should be created and loaded for such small feature. |
dunglas commentedAug 10, 2016 • 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.
The problem can be summarized by this tweet:https://twitter.com/lsmith/status/763074867919028224 Having a feature in the core (or in an officially supported bundle) instead of in a third-party, community maintained, repository makes a huge difference in term of marketing. Laravel has a feature like this one builtin, Laravel has a CQRS-like system builtin, Laravel has autowiring (and we now have it in core too - that makes a difference, some discussions appeared on Reddit PHP recently mentioning that Symfony now has autowiring builtin too). Symfony is the undisputed leader in the field of PHP middlewares. It competes with J2EE and is the foundation of high-quality projects including Laravel itself, Drupal and many more. That's great. But Symfony also has everything to be at the same time an excellent RAD framework. It only lacks some marketing, branding, official support and advertising. Importing such feature in core will help. |
nilportugues commentedAug 10, 2016
This is no minor discussion but I agree on@dunglas' points. If for instance, is a matter of rebranding this bundle, do so. Having an edition for this is a no-go as it is just causing pointless fragmentation. |
dunglas commentedJan 15, 2017
See#1034 |
Alternative to#959.
As "actions" and
__invokeseems to be confusing and a too big step fo now, let's focus on the essential for now: automatic dependency building.