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

Autoload the App namespace if none is configured #22

Merged
merged 2 commits into from
Jan 24, 2020

Conversation

aschempp
Copy link
Member

@aschempp aschempp commented Jan 8, 2020

With this change, we can remove https://github.com/contao/managed-edition/blob/master/composer.json#L35-L39

We could even make this more universal and use the plugin to set the asset path etc, so the default composer.json gets even smaller 😎

Copy link
Member

@Toflar Toflar left a comment

Choose a reason for hiding this comment

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

Needs tests but 👍

@leofeyer
Copy link
Member

leofeyer commented Jan 9, 2020

As I have stated in contao/contao#1165, I do not think that we should add any autoloading magic and stick to the Composer defaults instead.

leofeyer pushed a commit to contao/contao that referenced this pull request Jan 17, 2020
Description
-----------

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.

Commits
-------

5e2af0b Automatically load services in src directory
f41488f Added unit tests
cd86250 CS
12cd852 Rename the loading script
b14a62f Re-use the defaults() object
f1c3070 Fix the coding style
e669b82 Fix the --prefer-lowest tests
486bf84 Hardcode the integration test plugins to fix bundle tests
8c4ded2 Merge remote-tracking branch 'origin/feature/autoloader' into feature/autoloader
aa1f3c9 Catch throwables as well
0de01d8 Fixed conflicts and minimum requirements
5d17476 Hardcode the integration test plugins to fix bundle tests
6bb19ed Fix the CI Windows workflow
199d092 Merge branch 'master' into feature/autoloader
leofeyer pushed a commit to contao/manager-bundle that referenced this pull request Jan 17, 2020
Description
-----------

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.

Commits
-------

5e2af0b1 Automatically load services in src directory
f41488f5 Added unit tests
cd862508 CS
12cd852a Rename the loading script
b14a62f9 Re-use the defaults() object
f1c30701 Fix the coding style
e669b828 Fix the --prefer-lowest tests
486bf841 Hardcode the integration test plugins to fix bundle tests
8c4ded27 Merge remote-tracking branch 'origin/feature/autoloader' into feature/autoloader
aa1f3c99 Catch throwables as well
0de01d8e Fixed conflicts and minimum requirements
5d174760 Hardcode the integration test plugins to fix bundle tests
6bb19ed7 Fix the CI Windows workflow
199d0926 Merge branch 'master' into feature/autoloader
@aschempp
Copy link
Member Author

as contao/contao#1165 has been merged, I think we can merge this too?

@aschempp aschempp requested a review from Toflar January 18, 2020 13:15
Copy link
Member

@Toflar Toflar left a comment

Choose a reason for hiding this comment

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

Doesn‘t harm at all and brings us flexibility👍

leofeyer added a commit to contao/managed-edition that referenced this pull request Jan 18, 2020
@leofeyer
Copy link
Member

I have removed the "autoload" section from the composer.json file of the Managed Edition in contao/managed-edition@6b6a70c.

@aschempp aschempp merged commit b25aee4 into master Jan 24, 2020
@aschempp aschempp deleted the feature/autoloader branch January 24, 2020 18:14
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