extract autoloading out from bootstrap #854

Tobion · 2015-09-17T01:54:44Z

Fixes https://github.com/symfony/symfony-standard/issues/788#issuecomment-98993898 and symfony/symfony#14776 (comment).

by default class aggregation is still used as it is probably beneficial by default for most people (no opcache at all or stats enabled)
People that do not want to use the aggregation, can simply comment the line. They do not have to rewrite code. E.g. when using opcache with opcache.validate_timestamps=0 in production there is no point in using the bootstrap.
No debugging hell anymore as the class aggregation is only done in in no-debug mode
All environments still use the same code path, i.e. load autoload first. The only difference is the inlusion of the aggregation.
This way we can also simplify the bootstrap generation in the distribution bundle in the next major version of it.
several imrprovements like using require instead of require_once for the autoloader to rely on the return value and include_once instead of require_once for the bootstrap as the app is also working without it

weaverryan · 2015-09-17T03:09:58Z

This makes a lot of sense to me - make autoload.php anything that's always shared between the front controllers, console and tests.

Pierstoval · 2015-09-17T06:40:01Z

👍 , I always wondered why the bootstrap and the autoload were "merged". In dev I don't use the bootstrap, even if it is longer to load.

Tobion · 2015-09-17T09:56:43Z

It would be safe to merge this in 2.3 actually.

fabpot · 2015-09-17T10:07:38Z

That introduces an infinite loop between the autoloader and the bootstrap file. So, this can only be merged with versions of the bootstrap file that does not have the require on the autoload file. Which means changing the distribution bundle first, making a release, and then change the requirement in composer here.

stof · 2015-09-17T10:32:46Z

and the change in the DistributionBundle also requires this, otherwise the autoloader is not loaded at all anymore.

But I'm -1 on this implementation: this would mean that phpunit would now be loading the bootstrap.php.cache file as it is common to have app/autoload.php loaded in the Bootstrap (which can break as some classes may have been loaded already by PHPUnit). We would have the same issue with the Behat Symfony2Extension (and there, I'm 100% sure that loading the bootstrap.php.cache file will break because of Symfony classes being loaded earlier in Behat as it uses several components).

If you want to split it, I think the front controller should be responsible for loading the autoloader and then the bootstrap file when wanted. This would solve many issues:

no infinite loop (when using the current version of the bundle, you just load vendor/autoload.php twice, but this file is meant to be loadable as many times as you want to get access to the ClassLoader instance), no need for synchronized updates of the bundle and the SE
easier disabling of the bootstrap file when debugging (http://symfony.com/doc/current/cookbook/debugging.html#disabling-the-bootstrap-file-and-class-caching): you just have to comment the line in the front controller rather than having to change it
easy way to load the autoloading without the bootstrap file for tests (the same way than today)

Tobion · 2015-09-17T11:22:48Z

That introduces an infinite loop between the autoloader and the bootstrap file.

It does not as the files are only included once

and the change in the DistributionBundle also requires this, otherwise the autoloader is not loaded at all anymore.

I don't see what you mean.

this would mean that phpunit would now be loading the bootstrap.php.cache file

This was already the case before. This is the reason why I included the bootstrap in the autoload.

Pierstoval · 2015-09-17T11:27:02Z

and the change in the DistributionBundle also requires this, otherwise the autoloader is not loaded at all anymore.

I don't see what you mean.

The bootstrap file is generated by the SensioDistributionBundle, so it seems it has to be updated according to this update.

stof · 2015-09-17T11:44:20Z

It does not as the files are only included once

in this case, it means that the return value is broken. If the file was already loaded and the inclusion is skipped, this would return true instead of the ClassLoader.

This was already the case before

no it was not forced. I know many projects using app/autoload.php as their PHPUnit bootstrap file, precisely to avoid loading the bootstrap.php.cache.

Tobion · 2015-09-17T12:18:44Z

no it was not forced. I know many projects using app/autoload.php as their PHPUnit bootstrap file, precisely to avoid loading the bootstrap.php.cache.

It still not forced, people can use whatever autoload file they see fit. This is just the default way, just as it was before.

If you want to split it, I think the front controller should be responsible for loading the autoloader and then the bootstrap file when wanted.

So how do you include the bootstrap file for phpunit then? Or do you suggest to never do it at all?

in this case, it means that the return value is broken

For whom is the return value broken? I don't see it. The new way has the return value correctly. And people using the old way, also still have the return value.

stof · 2015-09-17T12:30:34Z

app/autoload.php

@@ -10,4 +12,6 @@

 AnnotationRegistry::registerLoader(array($loader, 'loadClass'));

+include_once __DIR__.'/bootstrap.php.cache';


should be require_once to get errors on missing files, not warnings.

why force errors? its optional. even if it cannot be included, the app still works. no need to require it

maybe we should even surpress the warning

stof · 2015-09-17T12:31:57Z

It still not forced, people can use whatever autoload file they see fit. This is just the default way, just as it was before.

but this would mean that they have to duplicate the autoload.php file, or switch the autoload.php file to the structure I'm suggesting to avoid loading the bootstrap.php.cache file

So how do you include the bootstrap file for phpunit then? Or do you suggest to never do it at all?

I don't include it. the bootstrap.php.cache file just duplicate some class definitions to perform a single loading for all of them instead of using autoloading. It is not necessary to use it (it is just a perf optimization). and not using it makes debugging easier.

For whom is the return value broken? I don't see it. The new way has the return value correctly. And people using the old way, also still have the return value.

web/app.php uses the return value of the file.
Actually, it is even more broken today. Something needing the return value should not use the _once variants of the function, and do it on a file allowing to include it multiple times. Otherwise it is unreliable.

also fix requiring the autoloader which must not use the _once variant because it would not be reliable to use the return value

Tobion · 2015-09-17T15:58:52Z

app/console

@@ -1,25 +1,30 @@
 #!/usr/bin/env php
 <?php

+use Symfony\Bundle\FrameworkBundle\Console\Application;
+use Symfony\Component\Console\Input\ArgvInput;
+use Symfony\Component\Debug\Debug;


to be consistent with the other front controllers where it is at the beginning

Tobion · 2015-09-17T16:03:44Z

I moved the boostrap into the frontcontrollers and only in debug mode. So it fully fixes #788

Tobion · 2015-09-17T16:06:42Z

web/app.php


 // Enable APC for autoloading to improve performance.
 // You should change the ApcClassLoader first argument to a unique prefix
 // in order to prevent cache key conflicts with other applications
 // also using APC.
 /*
-$apcLoader = new ApcClassLoader(sha1(__FILE__), $loader);
+$apcLoader = new Symfony\Component\ClassLoader\ApcClassLoader(sha1(__FILE__), $loader);


lets please not discuss this

hacfi · 2015-09-17T16:08:09Z

The new version of the PR looks much better to me! autoload.php shouldn’t have side-effects like including the bootstrap cache.

weaverryan · 2015-09-17T16:16:10Z

web/app_dev.php

+/**
+ * @var Composer\Autoload\ClassLoader $loader
+ */
+$loader = require __DIR__.'/../app/autoload.php';


Once upon a time, fabpot didn't want the front controllers to work differently. In practice, I like what you've done here. But we would need to be pretty much absolutely sure that there could never be (and has never been any reported) differences between these. Can we be that sure?

I don't see what difference you mean that could be related to this line? They all load autoload.php.

The difference is including bootstrap.php.cache in app.php and autoload.php in app_dev.php. You're right that both ultimately include app/autoload.php, but the risk (if it's even real) is that your bootstrap.php.cache is somehow out of sync with your source files, so things work in app_dev.php (with the true source files), but don't in app.php (which uses the compiled versions). Practically speaking, vendor files are only updated with Composer, which includes the script to re-update bootstrap.php.cache, so I'm not sure it's a real issue.

But, ezpublish, for example, already does this (loading a different file based on the environment): https://github.com/ezsystems/ezpublish-community/blob/master/web/index.php#L24. If they haven't had issues, I would think we also would be ok.

fabpot · 2015-09-23T07:47:04Z

app/console

 $input = new ArgvInput();
 $env = $input->getParameterOption(array('--env', '-e'), getenv('SYMFONY_ENV') ?: 'dev');
 $debug = getenv('SYMFONY_DEBUG') !== '0' && !$input->hasParameterOption(array('--no-debug', '')) && $env !== 'prod';

 if ($debug) {
    Debug::enable();
+} else {
+    include_once __DIR__.'/bootstrap.php.cache';


I would even not include the bootstrap for the console as it is less important anyway (and as you removed it from phpunit as well, that's not inconsistent either).

fabpot · 2015-09-23T07:50:25Z

Thank you @Tobion.

@fabpot

This PR was merged into the 5.0.x-dev branch. Discussion ---------- remove autoloading from bootstrap file Continuation of symfony/symfony-standard#854 to remove the obsolete mix of bootstrap and autoloading. @fabpot it would be good to merge this in 5.0, even if you just created the first relase of it. 5.x is also the dependency in the standard edition now. Commits ------- d1148b7 remove autoloading from bootstrap file

extract autoloading out from bootstrap

adc9564

Tobion mentioned this pull request Sep 17, 2015

Silence deprecation notices sensiolabs/SensioDistributionBundle#201

Merged

stof reviewed Sep 17, 2015
View reviewed changes

add bootstrap loading to frontcontrollers and only in debug mode

e7b8113

also fix requiring the autoloader which must not use the _once variant because it would not be reliable to use the return value

Tobion reviewed Sep 17, 2015
View reviewed changes

weaverryan reviewed Sep 17, 2015
View reviewed changes

Tobion mentioned this pull request Sep 22, 2015

Update SE for Symfony 3.0 #850

Merged

fabpot reviewed Sep 23, 2015
View reviewed changes

fabpot closed this in 0e8094a Sep 23, 2015

< 9E88 img class="avatar avatar-user" src="https://avatars.githubusercontent.com/u/610090?s=40&u=fedb67a3abf84ae167c43d49405b83ddb9ac5682&v=4" width="20" height="20" alt="@Tobion" /> Tobion deleted the extract-autoloading-from-bootstrap branch September 23, 2015 11:26

DavidBadura mentioned this pull request Nov 11, 2015

take over symfony changes DavidBadura/symfony-standard-edition#8

Closed

3 tasks

Tobion mentioned this pull request Nov 25, 2015

remove autoloading from bootstrap file sensiolabs/SensioDistributionBundle#242

Merged

xabbuh mentioned this pull request Nov 27, 2015

Fatal error: Interface 'Symfony\Component\HttpKernel\HttpKernelInterface' not found symfony/symfony#16713

Closed

jakzal mentioned this pull request Nov 29, 2015

bootstrap.php.cache not build correctly symfony/symfony#16738

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

extract autoloading out from bootstrap #854

extract autoloading out from bootstrap #854

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

		@@ -10,4 +12,6 @@

		AnnotationRegistry::registerLoader(array($loader, 'loadClass'));

		include_once __DIR__.'/bootstrap.php.cache';

Uh oh!

extract autoloading out from bootstrap #854

extract autoloading out from bootstrap #854

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!