Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Automatically load services in the src/ directory #1165

Merged
merged 14 commits into from
Jan 17, 2020
Merged

Conversation

aschempp
Copy link
Member

@aschempp aschempp commented Jan 8, 2020

This will finally add automatic autoloading for classes in the src folder. Due to Symfony's handling, it's a little bit complex. Here's how it works:

  • Do nothing, if any existing App\ service definition is available (e.g. in the config/services.yml)
  • Otherwise, try to register the services directory. If a class is not found (a reflection class cannot be created, because the namespace is not registered in Composer), Symfony will throw an exception. Only the PHP loader implementation is able to catch these exceptions, that's why we use an autoload.php and not a YAML file.
  • If services are successfully loaded, but the class does not exist in the file, the service definition will have an error (reflection exception, class found in autoloader but not in file).
  • If all new services have errors, it means there is no class with App\ namespace in the src/ folder, but probably something else (e.g. src/AppBundle/…). Then remove all services and ignore all errors.
  • If however only some services have errors, we assume the user has made a mistake, e.g. misspelled the class name. We do want to show these errors then.

We might discuss whether we want to automatically tag controllers like Symfony does: https://github.com/symfony/recipes/blob/master/symfony/framework-bundle/4.2/config/services.yaml#L22
I don't really see that point though, you can either extend AbstractController or tag the service yourself through annotations – something that's possible in Contao but not in Symfony (thanks to the ServiceAnnotationBundle 😉 )

This PR is complemented by contao/manager-plugin#22 which makes sure the App\ namespace is always registered in Composer. That's already the case for all regular managed edition because it was in the distribution composer.json, but it shouldn't be necessary to work.

@aschempp aschempp added this to the 4.9 milestone Jan 8, 2020
@aschempp aschempp self-assigned this Jan 8, 2020
@leofeyer leofeyer modified the milestones: 4.9, 4.10 Jan 9, 2020
@leofeyer leofeyer added the up for discussion Issues and PRs which will be discussed in our monthly Mumble calls. label Jan 9, 2020
@leofeyer
Copy link
Member

leofeyer commented Jan 9, 2020

This one is up for discussion, because I do not think that we should add any autoloading/autowiring magic besides the existing, well known and well documented Symfony defaults that you can enable in your services.yml file.

@fritzmg
Copy link
Contributor

fritzmg commented Jan 9, 2020

This one is up for discussion, because I do not think that we should add any autoloading/autowiring magic besides the existing, well known and well documented Symfony defaults that you can enable in your services.yml file.

What do you think about adding a pre-configured services.yml to the manager bundle skeleton, like I originally suggested?

@leofeyer
Copy link
Member

leofeyer commented Jan 9, 2020

Where did you suggest this?

@fritzmg
Copy link
Contributor

fritzmg commented Jan 9, 2020

Where did you suggest this?

Here: contao/managed-edition#42

I accidentally opened it in the wrong repository back then.

@leofeyer
Copy link
Member

leofeyer commented Jan 9, 2020

I like it better. 🙈

@aschempp
Copy link
Member Author

aschempp commented Jan 9, 2020

That won't work. If there is no src folder, or it contains anything else (e.g. an AppBundle), the container building will fail.

@aschempp
Copy link
Member Author

aschempp commented Jan 9, 2020

But implementing contao/managed-edition#42 is actually the point of this PR 😉

@fritzmg
Copy link
Contributor

fritzmg commented Jan 9, 2020

That won't work. If there is no src folder, or it contains anything else (e.g. an AppBundle), the container building will fail.

True, but the skeleton could also contain a src/.gitkeep ... 😬. And if you create your own local bundle after running composer create-project contao/managed-edition you're likely to adjust the services.yml to your needs anyway.

That being said, I think I am more in favor of this PR now 😁

But implementing contao/managed-edition#42 is actually the point of this PR 😉

👍

We might discuss whether we want to automatically tag controllers like Symfony does: https://github.com/symfony/recipes/blob/master/symfony/framework-bundle/4.2/config/services.yaml#L22
I don't really see that point though, you can either extend AbstractController or tag the service yourself through annotations – something that's possible in Contao but not in Symfony (thanks to the ServiceAnnotationBundle 😉 )

Personally, in my projects, I use the ADR pattern and regular dependency injection via constructor and thus and only include the following:

App\Action\:
    resource: ../src/Action
    public: true
namespace App\Action\ExampleAction;

use Doctrine\DBAL\Connection;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\Response;
use Symfony\Component\Routing\Annotation\Route;

/**
 * @Route("/example")
 */
class ExampleAction
{
    private $db;

    public function __construct(Connection $db)
    {
        $this->db = $db;
    }

    public function __invoke(Request $request): Response
    {
        return new Response('Hello World!');
    }
}

No tags needed. Service only needs to be public. May be you could add that? This way is also described in the getting started section of the documentation currently 😁

@leofeyer leofeyer removed the up for discussion Issues and PRs which will be discussed in our monthly Mumble calls. label Jan 16, 2020
@leofeyer leofeyer modified the milestones: 4.10, 4.9 Jan 16, 2020
@@ -216,6 +216,10 @@ public function registerContainerConfiguration(LoaderInterface $loader): void
if ($servicesFile = $this->getConfigFile('services.yml')) {
$loader->load($servicesFile);
}

if (is_dir($this->getProjectDir().'/src')) {
Copy link
Member

Choose a reason for hiding this comment

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

This condition is also true when unit testing the manager-bundle separately, therefore the bundles test fails: https://github.com/contao/contao/pull/1165/checks?check_run_id=393653023#step:4:888

Copy link
Member Author

Choose a reason for hiding this comment

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

When testing the bundle selaratey, its dependencies should still be installed? So core-bundle should be there as well as SecurityBundle ??

leofeyer
leofeyer previously approved these changes Jan 17, 2020
@leofeyer leofeyer merged commit 0408946 into master Jan 17, 2020
@leofeyer leofeyer deleted the feature/autoloader branch January 17, 2020 13:02
@leofeyer
Copy link
Member

Thank you @aschempp.

@leofeyer leofeyer changed the title Automatically load services in src directory Automatically load services in the src/ directory Jan 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants