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

Improve PSR-4 autoloading by default #1048

Merged
merged 1 commit into from
Jun 16, 2017
Merged

Improve PSR-4 autoloading by default #1048

merged 1 commit into from
Jun 16, 2017

Conversation

Pierstoval
Copy link
Contributor

@Pierstoval Pierstoval commented Feb 28, 2017

TL;DR: (updated Marth 3rd)
Before, I proposed to use any optimization by default in composer.json (complementary to PSR-4 autoload for the AppBundle).

I took note of the different arguments and removed loaders optimization because it is not suitable for dev environments, but kept AppBundle PSR-4 explicit load because it encourages PSR-4 best practices of autoload.


As seen on symfony/demo#490 (and it's certainly a well-known issue for developers using PHP Inspections EA Extended plugin for PHPStorm), having empty namespaces in psr-4 rules has an impact on performances.

Using this by default on the Standard Edition may encourage developers to add PSR-4 rules if they create new namespaces (for example if they want to create bundleless apps or other bundles, or components inside the same app, etc.).

What do you think about putting this in the SE?

Also, for best autoloading performances, it is recommended to use classmap-authoritative options (which adds optimize-autoloader implicitly). I added it in the composer.json but I'm not sure whether it's the best option to propose to Symfony newcomers (because composer install and update takes more time to execute).

For this have a question: should we recommend classmap-authoritative or optimize-autoloader (or nothing ❔) by default ?

@stof
Copy link
Member

stof commented Feb 28, 2017

Optimizing the autoloader should not be in the project-level composer.json. This is not a setting that you want to use in development. classmap-authoritative is particularly annoying in dev as it requires redumping the autoloader each time you add a class). optimize-autoloader is a bit better, but it still lead to weird errors when you delete a class (as the composer classloader will still assume the file exist and will try to require it when asking for the class).

Such settings should be turned on only in prod environments (and classmap-authoritative requires makign sure that your project works fine with it, which may not always be the case depending of what your code does)

@stof
Copy link
Member

stof commented Feb 28, 2017

To be clear, classmap-authoritative should be used in prod as soon as you can. And optimize-autoloader is just a must-have in prod. Not using it means you don't care about performance at all, as it is a performance boost without drawback (it has drawbacks in dev, but not in prod)

@Pierstoval
Copy link
Contributor Author

So you recommend nor classmap-authoritative nor optimize-autoloader for the SE?

@stof
Copy link
Member

stof commented Feb 28, 2017

@Pierstoval I never recommend putting them in the project itself (unless you never develop in it).
this setting should either be enabled in the system-level config on prod server (in $COMPOSER_HOME/config.json) or through the CLI flags in your deployment scripts.

@Pierstoval
Copy link
Contributor Author

@stof:

optimize-autoloader is a bit better, but it still lead to weird errors when you delete a class (as the composer classloader will still assume the file exist and will try to require it when asking for the class).

I find this argument very interesting: if you delete a class, it shouldn't be used in your application, therefore the class not found error would still be triggered by the autoloader, wouldn't it?

In this case, it means that optimize-autoloader would trigger errors only for mistakes that are already handled without optimizing the autoloader, so I would still suggest we may use it by default (instead of classmap-authoritative that sounds more production-specific)

@stof
Copy link
Member

stof commented Feb 28, 2017

@Pierstoval there are case like class_exists where passing a non-existent class to the autoloader is expected.
An outdated optimized autoloader will not be silent in this case, but will require the removed file.

@Pierstoval
Copy link
Contributor Author

Hmm... That makes me re-think about default autoload optimization in app. I may remove this part in this PR if it sounds too much for the SE.

By the way, what about the psr-4 changes? 😃

@bocharsky-bw
Copy link
Contributor

I suppose composer.lock should be updated.

@Pierstoval
Copy link
Contributor Author

I removed the classmap-authoritative part and updated the lock file hash

@stof
Copy link
Member

stof commented Mar 7, 2017

Composer now has a documentation page about optimizing the autoloader for production: https://getcomposer.org/doc/articles/autoloader-optimization.md

@Pierstoval Pierstoval changed the title Improve autoload performances by default Improve PSR-4 autoloading by default Mar 7, 2017
@Pierstoval
Copy link
Contributor Author

@stof Great! Then, I guess the Symfony documentation about deployment best practices can also link this article from Composer's website, and we can still review explicit PSR-4 autoload rule for AppBundle, I tend to continue thinking it's a good practice 😃

@stof
Copy link
Member

stof commented Mar 7, 2017

@Pierstoval linking to the composer doc from the Symfony one is the plan, yes. But we wanted to have the source of truth in the composer doc itself (as it is not specific to Symfony)

@Pierstoval
Copy link
Contributor Author

Then this PR needs to be reviewed 😸

@fabpot
Copy link
Member

fabpot commented Jun 16, 2017

Thank you @Pierstoval.

@fabpot fabpot merged commit 5f7ef8c into symfony:2.7 Jun 16, 2017
fabpot added a commit that referenced this pull request Jun 16, 2017
This PR was merged into the 2.7 branch.

Discussion
----------

Improve PSR-4 autoloading by default

TL;DR: (updated Marth 3rd)
Before, I proposed to use any optimization by default in `composer.json` (complementary to PSR-4 autoload for the AppBundle).

I took note of the different arguments and removed loaders optimization because it is not suitable for dev environments, but kept AppBundle PSR-4 explicit load because it encourages PSR-4 best practices of autoload.

---

As seen on symfony/demo#490 (and it's certainly a well-known issue for developers using PHP Inspections EA Extended plugin for PHPStorm), having empty namespaces in `psr-4` rules has an impact on performances.

Using this by default on the Standard Edition may encourage developers to add PSR-4 rules if they create new namespaces (for example if they want to create bundleless apps or other bundles, or components inside the same app, etc.).

What do you think about putting this in the SE?

Also, for best autoloading performances, it is recommended to use `classmap-authoritative` options (which adds `optimize-autoloader` implicitly). I added it in the `composer.json` but I'm not sure whether it's the best option to propose to Symfony newcomers (because `composer install` and `update` takes more time to execute).

For this have a question: should we recommend `classmap-authoritative` or `optimize-autoloader` (or nothing ❔) by default ?

Commits
-------

5f7ef8c Improve PSR-4 autoload by default
@Pierstoval Pierstoval deleted the 2.7 branch June 17, 2017 11:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants