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
Contributor

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 the feature label Jan 8, 2020
@aschempp aschempp added this to the 4.9 milestone Jan 8, 2020
@aschempp aschempp requested review from ausi, Toflar, sheeep, bytehead, leofeyer and fritzmg Jan 8, 2020
@aschempp aschempp self-assigned this Jan 8, 2020
@leofeyer leofeyer modified the milestones: 4.9, 4.10 Jan 9, 2020
@leofeyer

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

leofeyer commented Jan 9, 2020

Where did you suggest this?

@fritzmg

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

leofeyer commented Jan 9, 2020

I like it better. 🙈

@aschempp

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor Author

aschempp commented Jan 9, 2020

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

@fritzmg

This comment has been minimized.

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 😁

aschempp added 2 commits Jan 9, 2020
CS
@leofeyer leofeyer modified the milestones: 4.10, 4.9 Jan 16, 2020
aschempp and others added 2 commits 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')) {

This comment has been minimized.

Copy link
@leofeyer

leofeyer Jan 16, 2020

Member

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

This comment has been minimized.

Copy link
@aschempp

aschempp Jan 17, 2020

Author Contributor

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

@leofeyer leofeyer merged commit 0408946 into master Jan 17, 2020
9 checks passed
9 checks passed
Coverage
Details
Coding Style
Details
PHP 7.2
Details
PHP 7.3
Details
PHP 7.4
Details
Prefer Lowest
Details
Bundles
Details
Windows
Details
codecov/project No report found to compare against
Details
@leofeyer leofeyer deleted the feature/autoloader branch Jan 17, 2020
@leofeyer

This comment has been minimized.

Copy link
Member

leofeyer commented Jan 17, 2020

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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.