Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings
This repository was archived by the owner on Nov 27, 2020. It is now read-only.

Integrate DunglasActionBundle#960

Closed

Conversation

@dunglas
Copy link
Member

Alternative to#959.
As "actions" and__invoke seems to be confusing and a too big step fo now, let's focus on the essential for now: automatic dependency building.

GuilhemN reacted with thumbs up emoji
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
Copy link
Contributor

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.

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Why not.

Copy link
MemberAuthor

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.

GuilhemN reacted with thumbs up emoji
Copy link
Member

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.

Copy link
Member

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 theaction andconsole keys 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 mentionedStill an autowiring_type definition to add in Symfony to avoid this - what
    does this refer to?
  • Would it be possible to avoid the specialaction-annotation route type altogether? In other words, extend@Route to 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@Route may 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@Route system) effectively "asks" the container for the service for an instance and it returns it.

Copy link
Member

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.

Copy link
Member

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.

Copy link
MemberAuthor

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:

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.

@dunglasdunglas changed the title[WIP] Integrate DunglasActionBundleIntegrate DunglasActionBundleMar 25, 2016
password:"%mailer_password%"
spool:{ type: memory }


Copy link
Member

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
Copy link
Member

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 aget() method in the controller is already magic for beginners, imagine what happends when seeing this automatic injection stuff (as we discussed on the hackday, magic is great to advocate the framework and when you understand what's going on, but it only causes confusion in the months/years between these two phases).

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 :)

COil reacted with thumbs up emoji

@fabpot
Copy link
Member

Just for the record, I'm also very against controllers as services since they were introduced. I still think this is an anti-pattern.

COil, BoShurik, lyrixx, javiereguiluz, mickaelandrieu, brud, docteurklein, Ma27, fesor, BigZ, and 8 more reacted with thumbs up emojilinaori, alcohol, and TomasVotruba reacted with thumbs down emoji

@dunglas
Copy link
MemberAuthor

@wouterj, as one of the main documenter of Symfony, it's important to convince you of benefits of this approach.

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 get() method in the controller is already magic for beginners, imagine what happends when seeing this automatic injection stuff

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:

  • TheController directory is a magic directory where controller classes must be put
  • Those classes usually (even if it's not mandatory) extend a Symfony specific class (the abstract controller)
  • To reference this class, you must learn an uncommon Symfony specific syntax (MyBundle:MyController:myAction with a lot of possible misunderstanding (for instance, I often forgot to remove the mandatoryAction suffix of controller methods when I configure the routing)
  • To register a service, you must create a Symfony specific config file (YAML or XML service definition)
  • To access this service, you need to learn the API of the Symfony specific container

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:

  • TheController directory is still a magic directory where controller classes must be put (nothing different here)
  • Those classes are just POPO, you can use a helper trait to inject common dependencies if you want to, but there is nothing magic here (at least, less than in the current solution)
  • To reference this class, you just use its FQN in the routing system, just use it's FQN (no Symfony specific syntax here). Even if we all know that is a trick (the service name is the same than class name), it's elegant and easy to learn for newcomers
  • To registerand access a service, just create a class anywhere in your project, inject it in the constructor of your controller, and you're done (no XML or YAML syntax to learn, no config file to write, intuitive approach already used in many other libraries including Laravel and PHP DI, and common in other programming languages too, no dependencies to Symfony specific things like the container or the abstract controller class)

IMO (but I can be wrong), it's far less concept to learn to dive into Symfony, and it's an improved developper experience:

  1. create your logic business in POPO as usual
  2. create a class in theController directory
  3. type hint dependencies you need in your controller, Symfony automatically give you access to an instance of it
  4. create any method returning aResponse instance and map it to a route

You have a working Symfony application.

How can it be easier?

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 :)

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).

olivierphi and brud reacted with thumbs up emoji

@dunglas
Copy link
MemberAuthor

@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.

TomasVotruba reacted with thumbs up emoji

@gnugat
Copy link

@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
Copy link
Contributor

@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.

There's a dedicated discussion about editions in symfony/symfony-installer#39

@gquemener
Copy link

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
Copy link
Contributor

@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
Copy link
Contributor

What do you think about instead adding a new config option to theFrameworkBundle reproducingDunglasActionBundle's config ?
It would be disabled by default, wouldn't force people to install a new bundle for such small feature and wouldn't change any convention.
It would not be focused on controllers (it looks like it was the main concern?) and people who would like to use it would have to explicitly choose the directories autowired.

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
Copy link
Member

Why would FrameworkBundle need Dunglas's bundle config, but not that of FOSRestBundle for instance?

@GuilhemN
Copy link
Contributor

@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
Copy link
Member

wouterj commentedAug 10, 2016
edited
Loading

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?

chalasr reacted with thumbs up emoji

@GuilhemN
Copy link
Contributor

GuilhemN commentedAug 10, 2016
edited
Loading

I don't think we need two interfaces for the same thing (the standardCommandInterface andCommandBus). It could be in the core but the current implementation would probably have to be deprecated to avoid having to support several interfaces doing the same thing.

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.
I still don't think this is the same: Command bus is a pattern and every developer uses its own patterns.
Imo the core should only contain features that a significant part of the users may use and that won't have to change frequently (in case another pattern becomes the standard for example).

@bocharsky-bw
Copy link
Contributor

What's problem simply to add this bundle manually with$ composer require and configure it? It takes a few minutes. I don't think it's a big deal. If it is - look atKnpLabs/KnpRadBundle. It provides rapid development with Symfony and has many cool features, but I don't think to have it out-of-the-box is a good idea.

chalasr reacted with thumbs up emoji

@GuilhemN
Copy link
Contributor

@bocharsky-bw I don't think an entire bundle should be created and loaded for such small feature.
I also think that features that can be used by a significant part of the community should be in the core (as long as they don't add too much complexity).
BTW when something is in the core we know it is well supported/debugged and that we can receive feedbacks when changing something.

@dunglas
Copy link
MemberAuthor

dunglas commentedAug 10, 2016
edited
Loading

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.

GuilhemN, Cydonia7, enleur, Babacooll, dborsatto, Ma27, nilportugues, JMLamodiere, and Baldinof reacted with thumbs up emoji

@nilportugues
Copy link

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
Copy link
MemberAuthor

See#1034

Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@dunglas@wouterj@fabpot@gnugat@Pierstoval@gquemener@GuilhemN@bocharsky-bw@nilportugues@weaverryan

[8]ページ先頭

©2009-2025 Movatter.jp