merged branch bamarni/compile-classes (PR #4694)
Commits -------26a1e0b
[ClassLoader] ordered ClassCollectionLoader writing to avoid redeclaration at runtime Discussion ---------- [HttpKernel] allowed classes to compile to be prepended I had an issue when registering JMSDIExtraBundle before the frameworkBundle, because the bundle is adding a class to compile wich extends a class to compile added by the frameworkbundle (https://github.com/schmittjoh/JMSDiExtraBundle/blob/master/HttpKernel/ControllerResolver.php#L39). In my kernel, the bundle was registered before the frameworkbundle, if it's the case, the class is writed before the symfony core class in the cache file, so it will trigger the autoloader to load the symfony core class, then we'll have a fatal error because we're declaring 2 times the same class. I'm suggesting to add a way to prepend the classes to compile added by an extension, this way we could force classes from the core bundle to pe prepended, and avoid this kind of error with other bundles adding classes extending core classes or implementing core interface. I've also added it to the frameworkbundle. --------------------------------------------------------------------------- by travisbot at 2012-07-01T12:32:28Z This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1750090) (merged a989d35c intoa1b73887
). --------------------------------------------------------------------------- by schmittjoh at 2012-07-01T12:45:46Z This doesn't sound really failure proof, better would be to resolve the compiled classes in a way that handles the dependencies gracefully regardless of the order that they are registered in. --------------------------------------------------------------------------- by bamarni at 2012-07-01T13:03:15Z yes that would be much cleaner, even if it's not perfect, I don't have any other examples, but I think in most of situations this error happens because of the frameworkbundle late registration? --------------------------------------------------------------------------- by bamarni at 2012-07-01T23:07:24Z I've added an automatic reordering as suggested. Now I don't get the error whatever the order of FrameworkBundle and JMSDIExtraBundle is. --------------------------------------------------------------------------- by vicb at 2012-07-02T10:02:33Z Does the algo really works ? Let's say I have the following hierarchy `A < B < C` and the classes added in the following order; `C, B, A`. I believe that the algo would return `B, A, C` instead of `A, B, C`. An other question: should the dependency resolution be implemented in the `ClassCollectionLoader` rather than in the compiler pass ? --------------------------------------------------------------------------- by vicb at 2012-07-02T10:23:11Z @bamarni could you confirm the issue with the algo ? If it is confirmed, do you have time to work on a fix ? --------------------------------------------------------------------------- by bamarni at 2012-07-02T11:05:53Z the algo looks correct, there is a loop checking dependencies and appending the parent classes at the begining so it should also work with A > B > C, but I'll check and add a unit test. You're right about the location it could belong there if we want a general fix, should I put it in the ClassLoader component? --------------------------------------------------------------------------- by vicb at 2012-07-02T11:28:14Z Yep please move the code to the ClassLoader. You could also add some cache mechanism: - ReflectionClass could be cached (they might be use mulitple times when some dependencies exist and are also used by the class loader) - May be you could also cache the hierarchy of the classes --------------------------------------------------------------------------- by bamarni at 2012-07-02T17:25:08Z @vicb : indeed there were something wrong with the algo when it needed to handle more than 1 level of dependency, it's fixed, I've added a few test case and it passes. Caching the dependencies found with reflection would be useful if people are dumping several files with some similar classes in the same process, but I don't see why we would need to cache the hierarchy? --------------------------------------------------------------------------- by vicb at 2012-07-02T18:29:06Z @bamarni I was referring to a local cache (ie local variable). - On the first iteration, the code instantiate a reflection class, - On other iteration you would instantiate again a reflection class for a subset of the classes - ... - In the end the former code instantiate again a reflection class for each class in order to get the file name, ... I was also thinking of an other algo: count the number of parent classes for each classes to include and order classes according to this number. This would be a single loop only, what do you think ? --------------------------------------------------------------------------- by bamarni at 2012-07-02T19:32:34Z hum good idea counting the parents is cleaner, even though it looks enough, would it also make sense to treat interfaces and classes separately? I'm thinking about a file with all the interfaces at the begining (ordered by number of parents desc), then the classes would be appended (ordered by number of parents desc too). --------------------------------------------------------------------------- by vicb at 2012-07-03T07:05:07Z There is no real benefit in making the interfaces appear first but you can do it if it doesn't make the code more complex. Please also add the `@throws` annotation to the methods. --------------------------------------------------------------------------- by bamarni at 2012-07-03T09:37:28Z @vicb : I've changed it to use the parents count, looks good to me. --------------------------------------------------------------------------- by bamarni at 2012-07-03T12:23:46Z changed --------------------------------------------------------------------------- by bamarni at 2012-07-03T12:42:29Z fixed
This commit is contained in:
commit
2335dd0d60
@ -19,6 +19,7 @@ namespace Symfony\Component\ClassLoader;
|
||||
class ClassCollectionLoader
|
||||
{
|
||||
static private $loaded;
|
||||
static private $baseClassesCountMap;
|
||||
|
||||
/**
|
||||
* Loads a list of classes and caches them in one big file.
|
||||
@ -61,6 +62,9 @@ class ClassCollectionLoader
|
||||
$time = filemtime($cache);
|
||||
$meta = unserialize(file_get_contents($metadata));
|
||||
|
||||
sort($meta[1]);
|
||||
sort($classes);
|
||||
|
||||
if ($meta[1] != $classes) {
|
||||
$reload = true;
|
||||
} else {
|
||||
@ -81,6 +85,9 @@ class ClassCollectionLoader
|
||||
return;
|
||||
}
|
||||
|
||||
// order classes to avoid redeclaration at runtime (class declared before its parent)
|
||||
self::orderClasses($classes);
|
||||
|
||||
$files = array();
|
||||
$content = '';
|
||||
foreach ($classes as $class) {
|
||||
@ -220,4 +227,49 @@ class ClassCollectionLoader
|
||||
|
||||
return $output;
|
||||
}
|
||||
|
||||
/**
|
||||
* Orders a set of classes according to their number of parents.
|
||||
*
|
||||
* @param array $classes
|
||||
*
|
||||
* @throws \InvalidArgumentException When a class can't be loaded
|
||||
*/
|
||||
static private function orderClasses(array &$classes)
|
||||
{
|
||||
foreach ($classes as $class) {
|
||||
if (isset(self::$baseClassesCountMap[$class])) {
|
||||
continue;
|
||||
}
|
||||
|
||||
try {
|
||||
$reflectionClass = new \ReflectionClass($class);
|
||||
} catch (\ReflectionException $e) {
|
||||
throw new \InvalidArgumentException(sprintf('Unable to load class "%s"', $class));
|
||||
}
|
||||
|
||||
// The counter is cached to avoid reflection if the same class is asked again later
|
||||
self::$baseClassesCountMap[$class] = self::countParentClasses($reflectionClass) + count($reflectionClass->getInterfaces());
|
||||
}
|
||||
|
||||
asort(self::$baseClassesCountMap);
|
||||
|
||||
$classes = array_intersect(array_keys(self::$baseClassesCountMap), $classes);
|
||||
}
|
||||
|
||||
/**
|
||||
* Counts the number of parent classes in userland.
|
||||
*
|
||||
* @param \ReflectionClass $class
|
||||
* @param integer $count If exists, the current counter
|
||||
* @return integer
|
||||
*/
|
||||
static private function countParentClasses(\ReflectionClass $class, $count = 0)
|
||||
{
|
||||
if (($parent = $class->getParentClass()) && $parent->isUserDefined()) {
|
||||
$count = self::countParentClasses($parent, ++$count);
|
||||
}
|
||||
|
||||
return $count;
|
||||
}
|
||||
}
|
||||
|
@ -12,9 +12,74 @@
|
||||
namespace Symfony\Component\ClassLoader\Tests;
|
||||
|
||||
use Symfony\Component\ClassLoader\ClassCollectionLoader;
|
||||
use Symfony\Component\ClassLoader\UniversalClassLoader;
|
||||
|
||||
class ClassCollectionLoaderTest extends \PHPUnit_Framework_TestCase
|
||||
{
|
||||
/**
|
||||
* @dataProvider getDifferentOrders
|
||||
*/
|
||||
public function testClassReordering(array $classes)
|
||||
{
|
||||
$loader = new UniversalClassLoader();
|
||||
$loader->registerNamespace('ClassesWithParents', __DIR__.'/Fixtures');
|
||||
$loader->register();
|
||||
|
||||
$expected = <<<EOF
|
||||
<?php
|
||||
|
||||
namespace ClassesWithParents
|
||||
{
|
||||
|
||||
interface CInterface {}
|
||||
}
|
||||
|
||||
|
||||
namespace ClassesWithParents
|
||||
{
|
||||
|
||||
class B implements CInterface {}
|
||||
}
|
||||
|
||||
|
||||
namespace ClassesWithParents
|
||||
{
|
||||
|
||||
class A extends B {}
|
||||
}
|
||||
|
||||
EOF;
|
||||
|
||||
$dir = sys_get_temp_dir();
|
||||
$fileName = uniqid('symfony_');
|
||||
|
||||
ClassCollectionLoader::load($classes, $dir, $fileName, true);
|
||||
$cachedContent = @file_get_contents($dir.'/'.$fileName.'.php');
|
||||
|
||||
$this->assertEquals($expected, $cachedContent);
|
||||
}
|
||||
|
||||
public function getDifferentOrders()
|
||||
{
|
||||
return array(
|
||||
array(array(
|
||||
'ClassesWithParents\\A',
|
||||
'ClassesWithParents\\CInterface',
|
||||
'ClassesWithParents\\B',
|
||||
)),
|
||||
array(array(
|
||||
'ClassesWithParents\\B',
|
||||
'ClassesWithParents\\A',
|
||||
'ClassesWithParents\\CInterface',
|
||||
)),
|
||||
array(array(
|
||||
'ClassesWithParents\\CInterface',
|
||||
'ClassesWithParents\\B',
|
||||
'ClassesWithParents\\A',
|
||||
)),
|
||||
);
|
||||
}
|
||||
|
||||
public function testFixNamespaceDeclarations()
|
||||
{
|
||||
$source = <<<EOF
|
||||
|
@ -0,0 +1,5 @@
|
||||
<?php
|
||||
|
||||
namespace ClassesWithParents;
|
||||
|
||||
class A extends B {}
|
@ -0,0 +1,5 @@
|
||||
<?php
|
||||
|
||||
namespace ClassesWithParents;
|
||||
|
||||
class B implements CInterface {}
|
@ -0,0 +1,5 @@
|
||||
<?php
|
||||
|
||||
namespace ClassesWithParents;
|
||||
|
||||
interface CInterface {}
|
Reference in New Issue
Block a user