Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Do not exit from loadClass() early #5783

Merged
merged 2 commits into from

4 participants

@weierophinney

In loadClass(), if the namespace/prefix matches, but the classfile is not
found, we were returning a boolean false immediately. However, if a more
specific namespace/prefix matches, but is later in the stack, we're
essentially ignoring it, making the class unreachable.

This patch removes the return false line in the foreach loop, allowing a
namespace registered later to match and potentially fulfill the autoloading.

There will be a performance impact, which means it's always better to
register the more specific namespace earlier.

@weierophinney weierophinney Do not exit from loadClass() early
In loadClass(), if the namespace/prefix matches, but the classfile is not
found, we were returning a boolean false immediately. However, if a more
specific namespace/prefix matches, but is later in the stack, we're
essentially ignoring it, making the class unreachable.

This patch removes the `return false` line in the `foreach` loop, allowing
a namespace registered later to match and potentially fulfill the autoloading.

There _will_ be a performance impact, which means it's always better to register
the more specific namespace earlier.
4de2950
tests/ZendTest/Loader/StandardAutoloaderTest.php
@@ -190,4 +190,12 @@ public function testCanTellAutoloaderToRegisterZendNamespaceAtInstantiation()
$this->assertAttributeEquals($expected, 'namespaces', $loader);
}
+ public function testWillLoopThroughAllNamespacesUntilMatchIsFoundWhenAutoloading()
+ {
+ $loader = new StandardAutoloader();
+ $loader->registerNamespace('ZendTest\Loader\TestAsset\Parent', __DIR__ . '/TestAsset/Parent');
+ $loader->registerNamespace('ZendTest\Loader\TestAsset\Parent\Child', __DIR__ . '/TestAsset/Child');
+ $loader->autoload('ZendTest\Loader\TestAsset\Parent\Child\Subclass');
@Ocramius Collaborator
Ocramius added a note

Can you assert on the return value? Can't really know if it was autoloaded by a different loader, no?

@weierophinney Owner

The fact that no PHP errors are raised is indicative of the fact that it did not attempt to include a file it could not. However, I can likely do an assertion against !== false to validate; will update with that shortly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@EvanDotPro
Collaborator

Just to clarify here for anyone who references back to this PR later, the performance impact mentioned by @weierophinney is only applicable if you fall into this scenario where you do in fact have a more specific, overlapping namespace registered later. This is a very, very specific edge-case and unlikely to affect any current applications in practice (they would have simply ran into this bug). :smile:

@ralphschindler ralphschindler referenced this pull request from a commit
@ralphschindler ralphschindler Merge PR #5783 to develop
Merge branch 'hotfix/standard-autoloader-stack' of git://github.com/weierophinney/zf2 into weierophinney-hotfix/standard-autoloader-stack

* 'hotfix/standard-autoloader-stack' of git://github.com/weierophinney/zf2:
  [#5783] Assertion on return value
  Do not exit from loadClass() early
2ae43e3
@ralphschindler ralphschindler referenced this pull request from a commit
@ralphschindler ralphschindler Merge PR #5783 to develop
Merge branch 'weierophinney-hotfix/standard-autoloader-stack' into develop

* weierophinney-hotfix/standard-autoloader-stack:
  [#5783] Assertion on return value
  Do not exit from loadClass() early
4741ba7
@ralphschindler ralphschindler merged commit 70cd8a5 into zendframework:develop
@ralphschindler ralphschindler added this to the 2.3.0 milestone
@ralphschindler ralphschindler self-assigned this
@weierophinney weierophinney deleted the weierophinney:hotfix/standard-autoloader-stack branch
@weierophinney weierophinney modified the milestone: 2.3.0
@weierophinney weierophinney referenced this pull request from a commit in zendframework/zend-loader
@weierophinney weierophinney [zendframework/zf2#5783] Assertion on return value
- per @Ocramius
967f5f7
@gianarb gianarb referenced this pull request from a commit in zendframework/zend-loader
@ralphschindler ralphschindler Merge PR zendframework/zf2#5783 to develop
Merge branch 'hotfix/standard-autoloader-stack' of git://github.com/weierophinney/zf2 into weierophinney-hotfix/standard-autoloader-stack

* 'hotfix/standard-autoloader-stack' of git://github.com/weierophinney/zf2:
  [zendframework/zf2#5783] Assertion on return value
  Do not exit from loadClass() early
3d70932
@gianarb gianarb referenced this pull request from a commit in zendframework/zend-loader
@ralphschindler ralphschindler Merge PR zendframework/zf2#5783 to develop
Merge branch 'weierophinney-hotfix/standard-autoloader-stack' into develop

* weierophinney-hotfix/standard-autoloader-stack:
  [zendframework/zf2#5783] Assertion on return value
  Do not exit from loadClass() early
0f7d635
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Feb 4, 2014
  1. @weierophinney

    Do not exit from loadClass() early

    weierophinney authored
    In loadClass(), if the namespace/prefix matches, but the classfile is not
    found, we were returning a boolean false immediately. However, if a more
    specific namespace/prefix matches, but is later in the stack, we're
    essentially ignoring it, making the class unreachable.
    
    This patch removes the `return false` line in the `foreach` loop, allowing
    a namespace registered later to match and potentially fulfill the autoloading.
    
    There _will_ be a performance impact, which means it's always better to register
    the more specific namespace earlier.
  2. @weierophinney
This page is out of date. Refresh to see the latest.
View
1  library/Zend/Loader/StandardAutoloader.php
@@ -302,7 +302,6 @@ protected function loadClass($class, $type)
if (file_exists($filename)) {
return include $filename;
}
- return false;
}
}
return false;
View
9 tests/ZendTest/Loader/StandardAutoloaderTest.php
@@ -190,4 +190,13 @@ public function testCanTellAutoloaderToRegisterZendNamespaceAtInstantiation()
$this->assertAttributeEquals($expected, 'namespaces', $loader);
}
+ public function testWillLoopThroughAllNamespacesUntilMatchIsFoundWhenAutoloading()
+ {
+ $loader = new StandardAutoloader();
+ $loader->registerNamespace('ZendTest\Loader\TestAsset\Parent', __DIR__ . '/TestAsset/Parent');
+ $loader->registerNamespace('ZendTest\Loader\TestAsset\Parent\Child', __DIR__ . '/TestAsset/Child');
+ $result = $loader->autoload('ZendTest\Loader\TestAsset\Parent\Child\Subclass');
+ $this->assertTrue($result !== false);
+ $this->assertTrue(class_exists('ZendTest\Loader\TestAsset\Parent\Child\Subclass', false));
+ }
}
View
17 tests/ZendTest/Loader/TestAsset/Child/Subclass.php
@@ -0,0 +1,17 @@
+<?php
+/**
+ * Zend Framework (http://framework.zend.com/)
+ *
+ * @link http://github.com/zendframework/zf2 for the canonical source repository
+ * @copyright Copyright (c) 2005-2014 Zend Technologies USA Inc. (http://www.zend.com)
+ * @license http://framework.zend.com/license/new-bsd New BSD License
+ */
+
+namespace ZendTest\Loader\TestAsset\Parent\Child;
+
+/**
+ * @group Loader
+ */
+class Subclass
+{
+}
View
0  tests/ZendTest/Loader/TestAsset/Parent/.placeholder
No changes.
Something went wrong with that request. Please try again.