Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Jloader prefix #1760

Closed
wants to merge 4 commits into from

3 participants

Gary A Mort Andrew Eddie Don Gilbert
Gary A Mort

Codefix

Fixes loadByPrefix autoloader[_load] so that it doesn't give up on the first match.

Ex

Prefix: Tree, Path stubs/tree
Prefix: TreeLeaf, Path stubs/tree/leaf
class_exists(TreeLeafLeaf)

Current behavior

Will try to load either file stubs/tree/leaf/leaf.php or stubs/tree/leaf/leaf/leaf,php - but not both. Which file is attempted depends on which Prefix was loaded last(?). Even if the first file does not exist, it will still be the only file attempted.

New behavior

Check to class_exists is made after loading and only if true will the autoloader return.

Unit Tests

Implemented testRegisterPrefix in order to add test for this codefix
Included regression test to ensure that for multiple matching prefixes, multiple files are included if required. This test should fail for older platform code and succeed on this code.

    // Regression check that multiple matching prefixes can load multiple files
    $this->assertTrue($fileTreeLeafLeafLoaded, 'Tests that included files not defining class do not abort load.');

All other tests should succeed regardless of Platform version.

garyamort added some commits
Gary A Mort garyamort Fix for JLoader to handle multiple Prefix matches. IE:
prefix: DemoControllers, components/com_demo/controllers
prefix: Demo, components/com_demo

The first match will be tried, the rest of the mathces will be skipped even if it fails to find a file

Wheras:
prefix: DemoControllers, components/com_demo/controllers
prefix: DemoControllers, components/com_demo

Both will be tried.   Same prefix, multiple paths, all paths tried.  Different prefixes but related, only one works.
d087066
Gary A Mort garyamort Amended fix to include a check on class_exists before returning so mu…
…ltiple matching files where one does not include the class definition will be used.
f1c3a66
Gary A Mort garyamort Added love for testRegisterPrefix in JLoader.
Includes regression test for changes made to the loader
34f871f
Gary A Mort garyamort Correcting class name for stubs/tree/leaf/leaf.php so it will be loaded. 3eb30a4
Andrew Eddie

@garyamort is this something you think CMS 3.1 will need/want?

Don Gilbert
Collaborator

I proposed something similar to the PHP-FIG to modify PSR-0 so that it only returns true once the class is loaded. I think this is a good patch, and would help the CMS. If @elinw or @dextercowley could confirm that they would like this in the CMS, then I think it's good to merge.

Andrew Eddie

Since there has been no response I'm going to close this one.

Andrew Eddie eddieajau closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Dec 16, 2012
  1. Gary A Mort

    Fix for JLoader to handle multiple Prefix matches. IE:

    garyamort authored
    prefix: DemoControllers, components/com_demo/controllers
    prefix: Demo, components/com_demo
    
    The first match will be tried, the rest of the mathces will be skipped even if it fails to find a file
    
    Wheras:
    prefix: DemoControllers, components/com_demo/controllers
    prefix: DemoControllers, components/com_demo
    
    Both will be tried.   Same prefix, multiple paths, all paths tried.  Different prefixes but related, only one works.
  2. Gary A Mort

    Amended fix to include a check on class_exists before returning so mu…

    garyamort authored
    …ltiple matching files where one does not include the class definition will be used.
  3. Gary A Mort

    Added love for testRegisterPrefix in JLoader.

    garyamort authored
    Includes regression test for changes made to the loader
  4. Gary A Mort
This page is out of date. Refresh to see the latest.
8 libraries/loader.php
View
@@ -561,7 +561,13 @@ private static function _autoload($class)
if (strpos($class, $prefix) === 0 && ($chr === strtoupper($chr)))
{
- return self::_load(substr($class, strlen($prefix)), $lookup);
+ if(self::_load(substr($class, strlen($prefix)), $lookup))
+ {
+ if (class_exists($prefix.$class))
+ {
+ return true;
+ }
+ }
}
}
130 tests/suites/unit/JLoaderTest.php
View
@@ -220,6 +220,29 @@ public function testLoad()
$this->assertThat(JLoader::load('JLoaderTest'), $this->isTrue(), 'Tests that a loaded class returns true.');
}
+
+ /**
+ * Regression Tests for the JLoader::load method.
+ *
+ * @return void
+ *
+ * @since 11.3
+ * @covers JLoader::load
+ */
+ public function testLoadRegression()
+ {
+ JLoader::discover('Shuttle', __DIR__ . '/stubs/discover2', true);
+
+ JLoader::load('ShuttleChallenger');
+
+ $this->assertThat(JLoader::load('ShuttleChallenger'), $this->isTrue(), 'Tests that the class file was loaded.');
+
+ $this->assertThat(defined('CHALLENGER_LOADED'), $this->isTrue(), 'Tests that the class file was loaded.');
+
+ $this->assertThat(JLoader::load('Mir'), $this->isFalse(), 'Tests that an unknown class is ignored.');
+
+ $this->assertThat(JLoader::load('JLoaderTest'), $this->isTrue(), 'Tests that a loaded class returns true.');
+ }
/**
* Test the JLoader::loadByNamespaceLowerCase method
* with lower case namespace and path.
@@ -523,16 +546,111 @@ public function testRegisterNamespaceException()
*
* @return void
*
- * @since 12.1
+ * @since 12.3
* @covers JLoader::registerPrefix
- * @todo Implement testRegisterPrefix().
*/
public function testRegisterPrefix()
{
- // Remove the following lines when you implement this test.
- $this->markTestIncomplete(
- 'This test has not been implemented yet.'
- );
+ // Reset the prefixes.
+ TestReflection::setValue('JLoader', 'prefixes', array());
+
+ // We unregister all loader functions if registered.
+ $this->unregisterLoaders();
+
+ // Register the Tree/Leaf prefix
+ $path = dirname(__FILE__) . '/stubs/tree';
+ JLoader::registerPrefix('Tree', $path);
+
+
+ // Register the Tree/Leaf prefix
+ $path = dirname(__FILE__) . '/stubs/tree/leaf';
+ JLoader::registerPrefix('TreeLeaf', $path);
+
+ // Register the prefix autoloader.
+ spl_autoload_register(array('JLoader', '_autoload'));
+
+ // Get the list of autoload functions.
+ $newLoaders = spl_autoload_functions();
+
+ $foundLoad = false;
+ $foundAutoload = false;
+ $foundLoadByNamespaceLowerCase = false;
+ $loadByNamespaceNaturalCase = false;
+ $loadByNamespaceMixedCase = false;
+
+ // We search the list of autoload functions to see if our methods are there.
+ foreach ($newLoaders as $loader)
+ {
+ if (is_array($loader) && $loader[0] === 'JLoader')
+ {
+ if ($loader[1] === 'load')
+ {
+ $foundLoad = true;
+ }
+
+ if ($loader[1] === '_autoload')
+ {
+ $foundAutoload = true;
+ }
+
+ if ($loader[1] === 'loadByNamespaceLowerCase')
+ {
+ $foundLoadByNamespaceLowerCase = true;
+ }
+
+ if ($loader[1] === 'loadByNamespaceNaturalCase')
+ {
+ $loadByNamespaceNaturalCase = true;
+ }
+
+ if ($loader[1] === 'loadByNamespaceMixedCase')
+ {
+ $loadByNamespaceMixedCase = true;
+ }
+ }
+ }
+
+ // check for the leaves
+ $foundTreeLeafClass = class_exists('TreeLeaf', true);
+ $foundMapleLeafClass = class_exists('TreeLeafMaple', true);
+ $foundOakLeafClass = class_exists('TreeLeafOak', true);
+
+
+ // check that all files were loaded
+ $fileTreeLeafLeafLeafLoaded = defined('LEAF_LEAF_LEAF_LOADED');
+ $fileTreeLeafLeafOakLoaded = defined('LEAF_LEAF_OAK_LOADED');
+ $fileTreeLeafLeafLoaded = defined('LEAF_LEAF_LOADED');
+ $fileTreeLeafMapleLoaded = defined('LEAF_MAPLE_LOADED');
+
+ // Assert the prefix loader is found.
+ $this->assertTrue($foundAutoload);
+
+ // Assert the Tree, TreeLeaf prefixes have been registered.
+ $prefixes = TestReflection::getValue('JLoader', 'prefixes');
+ $this->assertArrayHasKey('Tree', $prefixes);
+ $this->assertArrayHasKey('TreeLeaf', $prefixes);
+
+ // Assert the leaf classes were found
+ $this->assertTrue($foundTreeLeafClass, 'Tests that the class file was loaded.');
+ $this->assertTrue($foundMapleLeafClass, 'Tests that the class file was loaded.');
+ $this->assertTrue($foundOakLeafClass, 'Tests that the class file was loaded.');
+
+ // Assert the leaf files were loaded
+ $this->assertTrue($fileTreeLeafLeafLeafLoaded, 'Tests that the correct file was loaded.');
+ $this->assertTrue($fileTreeLeafLeafOakLoaded, 'Tests that the correct file was loaded');
+ $this->assertTrue($fileTreeLeafMapleLoaded, 'Tests that the correct file was loaded.');
+
+ // Regression check, ver <12.2 will only attempt to load the first matching pattern regardless of whether the class was defined or not
+ $this->assertTrue($fileTreeLeafLeafLoaded, 'Tests that included files not defining class do not abort load.');
+
+
+ // Assert the class map loader is found.
+ $this->assertFalse($foundLoad);
+
+ // Assert the namespace loaders are not found.
+ $this->assertFalse($foundLoadByNamespaceLowerCase);
+ $this->assertFalse($loadByNamespaceNaturalCase);
+ $this->assertFalse($loadByNamespaceMixedCase);
}
/**
30 tests/suites/unit/stubs/tree/leaf/leaf.php
View
@@ -0,0 +1,30 @@
+<?php
+/**
+ * @package Joomla.UnitTest
+ *
+ * @copyright Copyright (C) 2005 - 2012 Open Source Matters, Inc. All rights reserved.
+ * @license GNU General Public License version 2 or later; see LICENSE
+ */
+
+/**#@+
+ * Constants
+ */
+/**
+ * LEAF_LOADED a constant to ensure this file was loaded
+ */
+define('LEAF_LEAF_LOADED', true);
+
+/**
+ * A lambda class to test prefix loader.
+ *
+ * @package Joomla.UnitTest
+ * @since 12.3
+ */
+abstract class TreeLeafLeaf
+{
+ static public function myDir()
+ {
+ return __DIR__;
+ }
+
+}
15 tests/suites/unit/stubs/tree/leaf/leaf/leaf.php
View
@@ -0,0 +1,15 @@
+<?php
+/**
+ * @package Joomla.Platform
+ *
+ * @copyright Copyright (C) 2005 - 2012 Open Source Matters, Inc. All rights reserved.
+ * @license GNU General Public License version 2 or later; see LICENSE
+ */
+
+/**#@+
+ * Constants
+ */
+/**
+ * LEAF_LEAF_LOADED a constant to ensure this file was loaded
+ */
+define('LEAF_LEAF_LEAF_LOADED', true);
29 tests/suites/unit/stubs/tree/leaf/leaf/oak.php
View
@@ -0,0 +1,29 @@
+<?php
+/**
+ * @package Joomla.UnitTest
+ *
+ * @copyright Copyright (C) 2005 - 2012 Open Source Matters, Inc. All rights reserved.
+ * @license GNU General Public License version 2 or later; see LICENSE
+ */
+
+/**#@+
+ * Constants
+ */
+/**
+ * LEAF_LOADED a constant to ensure this file was loaded
+ */
+define('LEAF_LEAF_OAK_LOADED', true);
+
+/**
+ * A lambda class to test prefix loader.
+ *
+ * @package Joomla.UnitTest
+ * @since 12.3
+ */
+abstract class TreeLeafOak
+{
+
+ static public function myDir() {
+ return __DIR__;
+ }
+}
29 tests/suites/unit/stubs/tree/leaf/maple.php
View
@@ -0,0 +1,29 @@
+<?php
+/**
+ * @package Joomla.UnitTest
+ *
+ * @copyright Copyright (C) 2005 - 2012 Open Source Matters, Inc. All rights reserved.
+ * @license GNU General Public License version 2 or later; see LICENSE
+ */
+
+/**#@+
+ * Constants
+ */
+/**
+ * LEAF_LOADED a constant to ensure this file was loaded
+ */
+define('LEAF_MAPLE_LOADED', true);
+
+/**
+ * A lambda class to test prefix loader.
+ *
+ * @package Joomla.UnitTest
+ * @since 12.3
+ */
+abstract class TreeLeafMaple
+{
+
+ static public function myDir() {
+ return __DIR__;
+ }
+}
Something went wrong with that request. Please try again.