Fix PSR4 in composer.json, add dump() #490

nicolas-grekas · 2017-02-27T15:00:29Z

Empty psr-4 prefixes can force the autoloader to scan the src/ directory for all namespaces. The best practice is to always make the prefix explicit.

composer update also, doing:

  - Updating twig/twig (v1.31.0 => v1.32.0) Loading from cache
  - Updating sensiolabs/security-checker (v4.0.0 => v4.0.1) Loading from cache
  - Updating phpunit/php-timer (1.0.8 => 1.0.9) Loading from cache
  - Updating phpunit/php-token-stream (1.4.9 => 1.4.11) Loading from cache

stof · 2017-02-27T15:04:14Z

👍

theofidry · 2017-02-27T15:11:52Z

composer.json

        "classmap": [ "app/AppKernel.php", "app/AppCache.php" ]
    },
    "autoload-dev": {
-        "psr-4": { "Tests\\": "tests/" }
+        "psr-4": { "Tests\\": "tests/" },
+        "files": [ "vendor/symfony/symfony/src/Symfony/Component/VarDumper/Resources/functions/dump.php" ]


why is this needed? isn't it supposed to already be included from the VarDumper composer.json?

This is more as to ask for clarifications. I seem to be unable to use the dump function when the app is not bootstrapped sometimes so I took the habit to add a shortcut to make a FQCN call to avoid that problem, but this still looks weird as I would expect to be able to use dump at anytime.

When symfony/symfony is required, the dump function is not autoloaded. It is only when symfony/var-dumper directly is.

Isn't the dump function loaded when Debug::enable() is called?

@Pierstoval no, it is loaded when the DebugBundle is booted. but when running tests, it is common for it not to be booted, making it painful. This is why we added it as a dev autoloading here for convenience.

Then symfony/symfony-standard#1025 should also be merged, shouldn't it? :)

javiereguiluz · 2017-02-27T15:12:04Z

Nice! I think this is important, so I've tweeted about it https://twitter.com/symfony_en/status/836232295329722370

This PR was merged into the master branch. Discussion ---------- Fix PSR4 in composer.json, add dump() Empty psr-4 prefixes can force the autoloader to scan the `src/` directory for all namespaces. The best practice is to always have make the prefix explicit. `composer update` also, doing: ``` - Updating twig/twig (v1.31.0 => v1.32.0) Loading from cache - Updating sensiolabs/security-checker (v4.0.0 => v4.0.1) Loading from cache - Updating phpunit/php-timer (1.0.8 => 1.0.9) Loading from cache - Updating phpunit/php-token-stream (1.4.9 => 1.4.11) Loading from cache ``` Commits ------- 3adc0b9 Fix PSR4 in composer.json, add dump()

theofidry · 2017-02-27T15:14:15Z

composer.json

@@ -4,11 +4,15 @@
    "type": "project",
    "description": "Symfony Demo Application",
    "autoload": {
-        "psr-4": { "": "src/" },
+        "psr-4": {


whilst this may boost performance, it may be harder for new users don't you think?

But only if you create a ton of bundles in your app ... which by the way is now considered a bad practice in most of the cases!

Well to be honest I don't even see why you need any bundle here in the first place, but it's not limited to bundles. It highly depends on how you manage your src bundle. I would personally promote more something like having a App namespace for the whole src.

@theofidry the symfony-demo project has a AppBundle namespace for its source, because this is what the Symfony best practices are telling (and the demo is also about showing these best practices).

The extra CodeExplorerBundle is more a project-specific vendor bundle actually (we haven't shared it as a real vendor bundle, because this feature simply does not make sense outside the demo project).

and enw users won't find it harder, as they won't need to change the autoload config of symfony-demo.

babaorum · 2017-02-27T15:48:16Z

First of all, nice catch !
But after a few minutes on the subject, don't you think this is just a patch and not really correcting the real issue ?
From my point of view we have composer on one hand which handle the autoloading.
And we have symfony on the other hand which handle the application.
But there is no way for symfony to tell composer where to look for our application directly.
So before your PR, composer was kind of on is own to find our app.
But now it's to the user to tell composer as well as symfony that we created a new bundle.

I think the idealist way would be that composer has a way to load this from an external file, which would be generated by symfony on cache warm up for prod env.

What do you think ? I would really appreciate your opinions on this.

Thanks by advance. And once more, great job for digging this.

nicolas-grekas · 2017-02-27T16:05:04Z

@babaorum my humble opinion: we can wonder how the world could be better with more things being done out of the box, but that's really hard, many tried already with some success (composer is great!), and that's always improving. If you think you have some ideas that could make all of this even better, PRs are welcomed on both Composer and Symfony. For this very PR, it's unrelated. Just my 2cts :)

juliendufresne · 2017-02-27T18:07:35Z

The issue only exists in the developper environment because when you push your code in production, you should use composer install --optimize-autoloader Or better composer install --classmap-authoritative which will produce a classmap and is the fastest autoload.

stof · 2017-02-28T09:41:38Z

@juliendufresne --optimize-autoloader will not help for classmap misses. So while it is already better (PSR-4 fallbacks are checked after PSR-4 prefixes but before any PSR-0 config), it is still better to use a prefix.
This optimization is indeed useless when using --classmap-authoritative as composer will not do any PSR-4 lookup at runtime then.

See: * https://twitter.com/symfony_en/status/836232295329722370 * symfony/demo#490 * https://github.com/symfony/symfony-standard/pull/1048/files

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

Fix PSR4 in composer.json, add dump()

3adc0b9

nicolas-grekas force-pushed the fix-composer branch from e1a9581 to 3adc0b9 Compare February 27, 2017 15:00

javiereguiluz added the FINISHED label Feb 27, 2017

theofidry reviewed Feb 27, 2017

View reviewed changes

javiereguiluz merged commit 3adc0b9 into symfony:master Feb 27, 2017

nicolas-grekas deleted the fix-composer branch February 27, 2017 15:12

theofidry reviewed Feb 27, 2017

View reviewed changes

Pierstoval mentioned this pull request Feb 28, 2017

Improve PSR-4 autoloading by default symfony/symfony-standard#1048

Merged

This was referenced Feb 28, 2017

Improve autoload performances by default campusdomar/PuMuKIT#331

Merged

Improve autoload performances campusdomar/PuMuKIT#335

Open

apetitpa mentioned this pull request Mar 15, 2017

Deploy to heroku fails #504

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix PSR4 in composer.json, add dump() #490

Fix PSR4 in composer.json, add dump() #490

nicolas-grekas commented Feb 27, 2017 •

edited

Loading

stof commented Feb 27, 2017

theofidry Feb 27, 2017

theofidry Feb 27, 2017

nicolas-grekas Feb 27, 2017 •

edited

Loading

Pierstoval Feb 28, 2017

stof Feb 28, 2017

Pierstoval Feb 28, 2017

javiereguiluz commented Feb 27, 2017

theofidry Feb 27, 2017

javiereguiluz Feb 27, 2017

theofidry Feb 27, 2017

stof Feb 27, 2017

babaorum commented Feb 27, 2017

nicolas-grekas commented Feb 27, 2017

juliendufresne commented Feb 27, 2017 •

edited

Loading

stof commented Feb 28, 2017

Fix PSR4 in composer.json, add dump() #490

Fix PSR4 in composer.json, add dump() #490

Conversation

nicolas-grekas commented Feb 27, 2017 • edited Loading

stof commented Feb 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas Feb 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javiereguiluz commented Feb 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

babaorum commented Feb 27, 2017

nicolas-grekas commented Feb 27, 2017

juliendufresne commented Feb 27, 2017 • edited Loading

stof commented Feb 28, 2017

nicolas-grekas commented Feb 27, 2017 •

edited

Loading

nicolas-grekas Feb 27, 2017 •

edited

Loading

juliendufresne commented Feb 27, 2017 •

edited

Loading