Skip to content
This repository was archived by the owner on Nov 27, 2020. It is now read-only.

Integrate DunglasActionBundle #960

Closed
wants to merge 3 commits into from
Closed

Integrate DunglasActionBundle #960

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f25a6b9
Integrate DunglasActionBundle
dunglas Mar 21, 2016
b2193a5
wip
dunglas Mar 23, 2016
5ae6916
Update deps
dunglas Mar 25, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions app/AppKernel.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ public function registerBundles()
new Symfony\Bundle\SwiftmailerBundle\SwiftmailerBundle(),
new Doctrine\Bundle\DoctrineBundle\DoctrineBundle(),
new Sensio\Bundle\FrameworkExtraBundle\SensioFrameworkExtraBundle(),
new Dunglas\ActionBundle\DunglasActionBundle(),
new AppBundle\AppBundle(),
];

Expand Down
12 changes: 12 additions & 0 deletions app/config/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,3 +66,15 @@ swiftmailer:
username: "%mailer_user%"
password: "%mailer_password%"
spool: { type: memory }

# Can be changed directly in the bundle is we found an agreement
dunglas_action:
autodiscover:
enabled: true
directories:
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 use symfony/finder in your bundle to be able to use something like ../src/*/Controller instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why not.

Copy link
Member Author

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 map Controller and Command without any risk to interfere with commands and controllers defined by public bundles.

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 across all bundles - I initially don't like that idea, and it complicates the configuration (I had to research what the difference was between the autodiscover and directories keys).

Also:

  • what causes console commands to autowire? Are the action and console keys significant? I think they are. If so, it's weird to me to also allow other keys - like foo (used in the bundle README). I understand that this is to allow other directories to be autowired, but I don't like that sometimes 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-annotation route type altogether? In other words, extend @Route to use a service definition for the controller if one exists? I ask because it feels like there are two ways to auto-register a controller as a service: either by using some config in config.yml or by using the action-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 we were to go to the trouble of supporting this, let's do it really 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 that action-annotation does nothing more than auto-set the _controller to the action.* pattern. That feels weak.

A more generic solution where @Route just works 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 that class 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
Member Author

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 for KernelInterface but it's not related anymore to this PR.



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

2 changes: 1 addition & 1 deletion app/config/routing.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
app:
resource: "@AppBundle/Controller/"
type: annotation
type: action-annotation
6 changes: 3 additions & 3 deletions bin/symfony_requirements
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ $lineSize = 70;
$symfonyRequirements = new SymfonyRequirements();
$iniPath = $symfonyRequirements->getPhpIniConfigPath();

echo_title('Symfony2 Requirements Checker');
echo_title('Symfony Requirements Checker');

echo '> PHP is using the following php.ini file:'.PHP_EOL;
if ($iniPath) {
Expand Down Expand Up @@ -43,9 +43,9 @@ foreach ($symfonyRequirements->getRecommendations() as $req) {
}

if ($checkPassed) {
echo_block('success', 'OK', 'Your system is ready to run Symfony2 projects');
echo_block('success', 'OK', 'Your system is ready to run Symfony projects');
} else {
echo_block('error', 'ERROR', 'Your system is not ready to run Symfony2 projects');
echo_block('error', 'ERROR', 'Your system is not ready to run Symfony projects');

echo_title('Fix the following mandatory requirements', 'red');

Expand Down
11 changes: 9 additions & 2 deletions composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,17 +10,24 @@
"autoload-dev": {
"psr-4": { "Tests\\": "tests/" }
},
"repositories": [
{
"type": "vcs",
"url": "https://github.com/dunglas/symfony"
}
],
"require": {
"php": ">=5.5.9",
"symfony/symfony": "3.1.*@dev",
"symfony/symfony": "dev-action as 3.1.0",
"doctrine/orm": "^2.5",
"doctrine/doctrine-bundle": "^1.6",
"doctrine/doctrine-cache-bundle": "^1.2",
"symfony/swiftmailer-bundle": "^2.3",
"symfony/monolog-bundle": "^2.8",
"sensio/distribution-bundle": "^5.0",
"sensio/framework-extra-bundle": "^3.0.2",
"incenteev/composer-parameter-handler": "^2.0"
"incenteev/composer-parameter-handler": "^2.0",
"dunglas/action-bundle": "dev-master"
},
"require-dev": {
"sensio/generator-bundle": "^3.0",
Expand Down
Loading