Skip to content
This repository

Fix UniversalClassLoader matching collisions. #43

Closed
wants to merge 1 commit into from

4 participants

Justin Hileman Fabien Potencier Paweł Skarżyński Jeremy Mikola
Justin Hileman

The current loadClass() implementation tries to load a class from the first matching prefix or namespace then stops, producing false-negative results. This is especially evident in groups of related libraries, such as Doctrine:

Doctrine
Doctrine\Common
Doctrine\Common\DataFixtures
Doctrine\DBAL
Doctrine\DBAL\Migrations

Each of these libraries is submoduled into a different vendor directory. Classes may or may not actually be loaded depending on what order these libraries are added to a UniversalClassLoader instance. This fix continues searching registered namespaces and prefixes if the first partial match is negative.

Justin Hileman bobthecow Fix UniversalClassLoader matching collisions.
The current `loadClass()` implementation tries to load a class from the first matching prefix then stops, producing false-negative results. This is especially evident in groups of related libraries, such as Doctrine:

    Doctrine
    Doctrine\Common
    Doctrine\Common\DataFixtures
    Doctrine\DBAL
    Doctrine\DBAL\Migrations

Each of these libraries is submoduled into a different vendor directory. Depending on what order these libraries are added to a UniversalClassLoader instance, classes may or may not actually be loaded. This fix continues searching registered namespaces and prefixes if the first partial match is negative.
54d20f2
Fabien Potencier
Owner

Orders matter, and I think this is a good thing. You must put the most precise namespace first. I don't want the autoloader to be able to load the same class from different directories as it can become a nightmare pretty fast. Or is there another reason?

Paweł Skarżyński

If there is another reason (than putting namespaces in random order), maybe a solution is as follows: current behaviour remain unchanged, but when in configuration appears a proper setting, the loader look at all directories?
e.g.
classloader_search_directories = true/false [default=false]

Fabien Potencier
Owner

Why make things more complicated?

Jeremy Mikola

Are you going to submit this as a pull request to fabpot/symfony?

I submitted it to symfony/symfony: symfony#43

Oh, I didn't realize this was to "solve" issues of ordering. So it doesn't appear that it will be accepted; we should probably bump our submodule back to new_master tomorrow. It's not a problem to ensure correct ordering in our namespace array - I've been doing that myself for some time :)

This is in a different branch. Our submodule has never pointed at this commit :)

Justin Hileman

This isn't about putting namespaces in a random order. It's about keeping UniversalClassLoader from doing anything unexpected. In Symfony2 configs, anything set later will overload earlier values. By this precedent, I would expect the following to work:

$loader = new UniversalClassLoader();
$loader->registerNamespaces(array(
    'Doctrine\\Common' => __DIR__.'/vendor/doctrine-common/lib',
    'Doctrine\\DBAL'   => __DIR__.'/vendor/doctrine-dbal/lib',
    'Zend'             => __DIR__.'/vendor/zend/library',
));
$loader->registerNamespaces(array(
    'Doctrine\\Common\\DataFixtures' => __DIR__.'/vendor/doctrine-data-fixtures/lib',
    'Doctrine\\DBAL\\Migrations'     => __DIR__.'/vendor/doctrine-migrations/lib',
));
$loader->register();

But of course it doesn't, because the set of namespaces registered earlier include two collisions. Surprisingly, the following won't work either:

$loader = new UniversalClassLoader();
$loader->registerNamespaces(array(
    'Doctrine\\Common' => __DIR__.'/vendor/doctrine-common/lib',
    'Doctrine\\DBAL'   => __DIR__.'/vendor/doctrine-dbal/lib',
    'Zend'             => __DIR__.'/vendor/zend/library',
));
$loader->registerNamespaces(array(
    'Doctrine\\Common\\DataFixtures' => __DIR__.'/vendor/doctrine-data-fixtures/lib',
    'Doctrine\\Common'               => __DIR__.'/vendor/doctrine-common/lib',
    'Doctrine\\DBAL\\Migrations'     => __DIR__.'/vendor/doctrine-migrations/lib',
    'Doctrine\\DBAL'                 => __DIR__.'/vendor/doctrine-dbal/lib',
));
$loader->register();

But this works just fine:

$loader = new UniversalClassLoader();
$loader->registerNamespaces(array(
    'Doctrine\\Common' => __DIR__.'/vendor/doctrine-common/lib',
    'Doctrine\\DBAL'   => __DIR__.'/vendor/doctrine-dbal/lib',
    'Zend'             => __DIR__.'/vendor/zend/library',
));
$loader->register();

$loader2 = new UniversalClassLoader();
$loader2->registerNamespaces(array(
    'Doctrine\\Common\\DataFixtures' => __DIR__.'/vendor/doctrine-data-fixtures/lib',
    'Doctrine\\Common'               => __DIR__.'/vendor/doctrine-common/lib',
    'Doctrine\\DBAL\\Migrations'     => __DIR__.'/vendor/doctrine-migrations/lib',
    'Doctrine\\DBAL'                 => __DIR__.'/vendor/doctrine-dbal/lib',
));
$loader2->register();

I'm not sure I understand your concern. This fix seems to be a net win. If you maintain the current "most precise first" namespaces, this will not be any slower or more complicated: it will return matches just as fast, and will never enter the fallback checks.

Fabien Potencier
Owner

merged.

Justin Hileman

Thanks!

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Showing 1 unique commit by 1 author.

Jan 03, 2011
Justin Hileman bobthecow Fix UniversalClassLoader matching collisions.
The current `loadClass()` implementation tries to load a class from the first matching prefix then stops, producing false-negative results. This is especially evident in groups of related libraries, such as Doctrine:

    Doctrine
    Doctrine\Common
    Doctrine\Common\DataFixtures
    Doctrine\DBAL
    Doctrine\DBAL\Migrations

Each of these libraries is submoduled into a different vendor directory. Depending on what order these libraries are added to a UniversalClassLoader instance, classes may or may not actually be loaded. This fix continues searching registered namespaces and prefixes if the first partial match is negative.
54d20f2
This page is out of date. Refresh to see the latest.
10 src/Symfony/Component/HttpFoundation/UniversalClassLoader.php
@@ -132,13 +132,12 @@ public function loadClass($class)
132 132 $namespace = substr($class, 0, $pos);
133 133 foreach ($this->namespaces as $ns => $dir) {
134 134 if (0 === strpos($namespace, $ns)) {
135   - $class = substr($class, $pos + 1);
136   - $file = $dir.DIRECTORY_SEPARATOR.str_replace('\\', DIRECTORY_SEPARATOR, $namespace).DIRECTORY_SEPARATOR.str_replace('_', DIRECTORY_SEPARATOR, $class).'.php';
  135 + $className = substr($class, $pos + 1);
  136 + $file = $dir.DIRECTORY_SEPARATOR.str_replace('\\', DIRECTORY_SEPARATOR, $namespace).DIRECTORY_SEPARATOR.str_replace('_', DIRECTORY_SEPARATOR, $className).'.php';
137 137 if (file_exists($file)) {
138 138 require $file;
  139 + return;
139 140 }
140   -
141   - return;
142 141 }
143 142 }
144 143 } else {
@@ -148,9 +147,8 @@ public function loadClass($class)
148 147 $file = $dir.DIRECTORY_SEPARATOR.str_replace('_', DIRECTORY_SEPARATOR, $class).'.php';
149 148 if (file_exists($file)) {
150 149 require $file;
  150 + return;
151 151 }
152   -
153   - return;
154 152 }
155 153 }
156 154 }
8 tests/Symfony/Tests/Component/HttpFoundation/Fixtures/alpha/NamespaceCollision/A/Bar.php
... ... @@ -0,0 +1,8 @@
  1 +<?php
  2 +
  3 +namespace NamespaceCollision\A;
  4 +
  5 +class Bar
  6 +{
  7 + public static $loaded = true;
  8 +}
8 tests/Symfony/Tests/Component/HttpFoundation/Fixtures/alpha/NamespaceCollision/A/Foo.php
... ... @@ -0,0 +1,8 @@
  1 +<?php
  2 +
  3 +namespace NamespaceCollision\A;
  4 +
  5 +class Foo
  6 +{
  7 + public static $loaded = true;
  8 +}
6 tests/Symfony/Tests/Component/HttpFoundation/Fixtures/alpha/PrefixCollision/A/Bar.php
... ... @@ -0,0 +1,6 @@
  1 +<?php
  2 +
  3 +class PrefixCollision_A_Bar
  4 +{
  5 + public static $loaded = true;
  6 +}
6 tests/Symfony/Tests/Component/HttpFoundation/Fixtures/alpha/PrefixCollision/A/Foo.php
... ... @@ -0,0 +1,6 @@
  1 +<?php
  2 +
  3 +class PrefixCollision_A_Foo
  4 +{
  5 + public static $loaded = true;
  6 +}
8 tests/Symfony/Tests/Component/HttpFoundation/Fixtures/beta/NamespaceCollision/A/B/Bar.php
... ... @@ -0,0 +1,8 @@
  1 +<?php
  2 +
  3 +namespace NamespaceCollision\A\B;
  4 +
  5 +class Bar
  6 +{
  7 + public static $loaded = true;
  8 +}
8 tests/Symfony/Tests/Component/HttpFoundation/Fixtures/beta/NamespaceCollision/A/B/Foo.php
... ... @@ -0,0 +1,8 @@
  1 +<?php
  2 +
  3 +namespace NamespaceCollision\A\B;
  4 +
  5 +class Foo
  6 +{
  7 + public static $loaded = true;
  8 +}
6 tests/Symfony/Tests/Component/HttpFoundation/Fixtures/beta/PrefixCollision/A/B/Bar.php
... ... @@ -0,0 +1,6 @@
  1 +<?php
  2 +
  3 +class PrefixCollision_A_B_Bar
  4 +{
  5 + public static $loaded = true;
  6 +}
6 tests/Symfony/Tests/Component/HttpFoundation/Fixtures/beta/PrefixCollision/A/B/Foo.php
... ... @@ -0,0 +1,6 @@
  1 +<?php
  2 +
  3 +class PrefixCollision_A_B_Foo
  4 +{
  5 + public static $loaded = true;
  6 +}
101 tests/Symfony/Tests/Component/HttpFoundation/UniversalClassLoaderTest.php
@@ -37,5 +37,104 @@ public static function testClassProvider()
37 37 array('\\Pearlike_Bar', '\\Pearlike_Bar', '->loadClass() loads Pearlike_Bar class with a leading slash'),
38 38 );
39 39 }
40   -}
41 40
  41 + /**
  42 + * @dataProvider namespaceCollisionClassProvider
  43 + */
  44 + public function testLoadClassNamespaceCollision($namespaces, $className, $message)
  45 + {
  46 + $loader = new UniversalClassLoader();
  47 + $loader->registerNamespaces($namespaces);
  48 +
  49 + $loader->loadClass($className);
  50 + $this->assertTrue(class_exists($className), $message);
  51 + }
  52 +
  53 + public static function namespaceCollisionClassProvider()
  54 + {
  55 + return array(
  56 + array(
  57 + array(
  58 + 'NamespaceCollision\\A' => __DIR__ . DIRECTORY_SEPARATOR . 'Fixtures/alpha',
  59 + 'NamespaceCollision\\A\\B' => __DIR__ . DIRECTORY_SEPARATOR . 'Fixtures/beta',
  60 + ),
  61 + 'NamespaceCollision\A\Foo',
  62 + '->loadClass() loads NamespaceCollision\A\Foo from alpha.',
  63 + ),
  64 + array(
  65 + array(
  66 + 'NamespaceCollision\\A\\B' => __DIR__ . DIRECTORY_SEPARATOR . 'Fixtures/beta',
  67 + 'NamespaceCollision\\A' => __DIR__ . DIRECTORY_SEPARATOR . 'Fixtures/alpha',
  68 + ),
  69 + 'NamespaceCollision\A\Bar',
  70 + '->loadClass() loads NamespaceCollision\A\Bar from alpha.',
  71 + ),
  72 + array(
  73 + array(
  74 + 'NamespaceCollision\\A' => __DIR__ . DIRECTORY_SEPARATOR . 'Fixtures/alpha',
  75 + 'NamespaceCollision\\A\\B' => __DIR__ . DIRECTORY_SEPARATOR . 'Fixtures/beta',
  76 + ),
  77 + 'NamespaceCollision\A\B\Foo',
  78 + '->loadClass() loads NamespaceCollision\A\B\Foo from beta.',
  79 + ),
  80 + array(
  81 + array(
  82 + 'NamespaceCollision\\A\\B' => __DIR__ . DIRECTORY_SEPARATOR . 'Fixtures/beta',
  83 + 'NamespaceCollision\\A' => __DIR__ . DIRECTORY_SEPARATOR . 'Fixtures/alpha',
  84 + ),
  85 + 'NamespaceCollision\A\B\Bar',
  86 + '->loadClass() loads NamespaceCollision\A\B\Bar from beta.',
  87 + ),
  88 + );
  89 + }
  90 +
  91 + /**
  92 + * @dataProvider prefixCollisionClassProvider
  93 + */
  94 + public function testLoadClassPrefixCollision($prefixes, $className, $message)
  95 + {
  96 + $loader = new UniversalClassLoader();
  97 + $loader->registerPrefixes($prefixes);
  98 +
  99 + $loader->loadClass($className);
  100 + $this->assertTrue(class_exists($className), $message);
  101 + }
  102 +
  103 + public static function prefixCollisionClassProvider()
  104 + {
  105 + return array(
  106 + array(
  107 + array(
  108 + 'PrefixCollision_A_' => __DIR__ . DIRECTORY_SEPARATOR . 'Fixtures/alpha',
  109 + 'PrefixCollision_A_B_' => __DIR__ . DIRECTORY_SEPARATOR . 'Fixtures/beta',
  110 + ),
  111 + 'PrefixCollision_A_Foo',
  112 + '->loadClass() loads PrefixCollision_A_Foo from alpha.',
  113 + ),
  114 + array(
  115 + array(
  116 + 'PrefixCollision_A_B_' => __DIR__ . DIRECTORY_SEPARATOR . 'Fixtures/beta',
  117 + 'PrefixCollision_A_' => __DIR__ . DIRECTORY_SEPARATOR . 'Fixtures/alpha',
  118 + ),
  119 + 'PrefixCollision_A_Bar',
  120 + '->loadClass() loads PrefixCollision_A_Bar from alpha.',
  121 + ),
  122 + array(
  123 + array(
  124 + 'PrefixCollision_A_' => __DIR__ . DIRECTORY_SEPARATOR . 'Fixtures/alpha',
  125 + 'PrefixCollision_A_B_' => __DIR__ . DIRECTORY_SEPARATOR . 'Fixtures/beta',
  126 + ),
  127 + 'PrefixCollision_A_B_Foo',
  128 + '->loadClass() loads PrefixCollision_A_B_Foo from beta.',
  129 + ),
  130 + array(
  131 + array(
  132 + 'PrefixCollision_A_B_' => __DIR__ . DIRECTORY_SEPARATOR . 'Fixtures/beta',
  133 + 'PrefixCollision_A_' => __DIR__ . DIRECTORY_SEPARATOR . 'Fixtures/alpha',
  134 + ),
  135 + 'PrefixCollision_A_B_Bar',
  136 + '->loadClass() loads PrefixCollision_A_B_Bar from beta.',
  137 + ),
  138 + );
  139 + }
  140 +}

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.