Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Namespaces support for JLoader #1591

Merged
merged 3 commits into from Nov 12, 2012

Conversation

Projects
None yet
5 participants
Contributor

florianv commented Oct 10, 2012

This pull request adds support for class loading and autoloading based on namespaces.

If your class is located in : BASE_PATH/Chess/Piece/Pawn.php :

<?php
namespace Chess\Piece;

class Pawn {} 

You can register it to the auto loader, by registering the ROOT namespace Chess:
JLoader::registerNamespace('Chess', BASE_PATH . '/Chess');

All classes respecting the naming convention in a given namespace :
namespace Folder\SubFolder; for classes in BASE_PATH/Folder/SubFolder/ will be autoloaded.

If you have lower case directory names and class names BASE_PATH/folder/subfolder/, you can either declarate the namespace with lower or camel cases, and it will work too.

But note that the first param of JLoader::registerNamespace is case sensitive and must match the namespace declaration case.

Examples: namespace Chess; JLoader::registerNamespace('Chess', PATH_TO_CHESS);

namespace chess; JLoader::registerNamespace('chess', PATH_TO_CHESS);

You can also register multiple lookup paths for a given namespace (like the prefix).

Contributor

LouisLandry commented Oct 11, 2012

@florianv would you please migrate the documentation in the description from #1400 over to this one so we have it all here? The change log is generated based on merged pull requests so it's nice to have it all in one place.

Also, I wonder if you'd entertain a thought I've had on JLoader. I think something like:

// Setup the Joomla Platform autoloader.
JLoader::setup($enableJimport, $enableNamespaces);

The idea being that those two boolean flags would decide whether we even add the different autoloader methods. For now we would need to default the first argument to true and the second to false, but it would let any application using the platform decide which of those are loaded. Hopefully we can get to a point in the near future where the first one is defaulted to false and we are reconsidering the second one.

@ianmacl ianmacl commented on an outdated diff Oct 11, 2012

tests/suites/unit/JLoaderTest.php
+ *
+ * @return void
+ *
+ * @since 12.3
+ * @covers JLoader::loadByNamespace
+ */
+ public function testLoadByNamespace()
+ {
+ // Try with a namespace matching the directory structure letter case.
+ $path = dirname(__FILE__) . '/stubs/Color';
+ JLoader::registerNamespace('Color', $path);
+
+ // Test with leading '\\'.
+ $this->assertTrue(JLoader::loadByNamespace('\\Color\\Rgb\\Red'));
+
+ // Try with a namespace containing upper case letters but lower case directory and file names.
@ianmacl

ianmacl Oct 11, 2012

Contributor

These should be broken up into different test methods so you can get a granular picture of what is breaking.

@ianmacl ianmacl commented on the diff Oct 11, 2012

tests/suites/unit/JLoaderTest.php
+ {
+ // Try with a valid path.
+ $path = dirname(__FILE__) . '/stubs/discover1';
+ JLoader::registerNamespace('discover', $path);
+
+ $namespaces = JLoader::getNamespaces();
+
+ $this->assertContains($path, $namespaces['discover']);
+
+ // Try to add an other path for the namespace.
+ $path = dirname(__FILE__) . '/stubs/discover2';
+ JLoader::registerNamespace('discover', $path);
+ $namespaces = JLoader::getNamespaces();
+
+ $this->assertCount(2, $namespaces['discover']);
+ $this->assertContains($path, $namespaces['discover']);
@ianmacl

ianmacl Oct 11, 2012

Contributor

Again, break these up.

Contributor

florianv commented Oct 11, 2012

Yes it is a good idea :)

What I was thinking before is eventually to have 2 loaders : JLoader and JLoaderNamespace having the same base.

<?php
interface JLoaderBase
{
  public function load($class);

  public function setup();

  // Something = prefix / namespace
  public function register($something, $path, $reset = false);
}

So people can set up the namespace one if they want to independently of the actual one.

Both solutions are correct I think, but yours implies less changes.

But, supposing in the future Joomla is fully namespaced and wants to get rid of the actual loader, I think maybe we would like not having param refering to the old one in setup().

Edit : But my solution is only valid if the loader keeps his name : JLoaderNamespace, when JLoader leaves. Otherwise people would have to modify their calls to it.

Contributor

florianv commented Oct 11, 2012

@ianmacl Thanks, I will split them asap.

Contributor

LouisLandry commented Oct 11, 2012

@florianv I think your solution is probably the more "academically correct" one. That being said, the class is static and the way it is currently used I am not sure that I think there'd be much benefit in changing that behavior. As for the parameter ordering I actually think I agree with you. How about we do something like:

<?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
 */

defined('JPATH_PLATFORM') or die;

/**
 * Static class to handle loading of libraries.
 *
 * @package  Joomla.Platform
 * @since    11.1
 */
abstract class JLoader
{
    // ...

    /**
     * Method to setup the autoloaders for the Joomla Platform.  Since the SPL autoloaders are
     * called in a queue we will add our explicit, class-registration based loader first, then
     * fall back on the autoloader based on conventions.  This will allow people to register a
     * class in a specific location and override platform libraries as was previously possible.
     *
     * @param   boolean  $allowMixedCasePaths  True to allow class paths to use mixed case paths that match class names.
     * @param   boolean  $enableNamespaces     True to enable PHP namespace based class autoloading.
     * @param   boolean  $enableJimport        True to enable legacy jimport() based library loading.
     *
     * @return  void
     *
     * @since   11.3
     */
    public static function setup($allowMixedCasePaths = false, $enableNamespaces = false, $enableJimport = true)

    // ...
}

// Setup the Joomla Platform autoloader.
JLoader::setup($enableJimport, $enableNamespaces);

This has the advantage of not forcing a change in method signature down the road. At some point the $enableJimport flag will be deprecated most likely so we would want it last. Similarly at some point we likely will only support namespace based class loading so the $enableNamespaces flag will be deprecated as well.

The one we end up keeping is the flag to allow the mixed case paths. I actually like that you have that option in there, but I'd rather not incur the cost of an extra filestat on every load if it is just completely unnecessary and all paths are lower case.

Does that make sense?

Contributor

florianv commented Oct 11, 2012

Yes it does. The $allowMixedCasePaths is important.

I don't know if you have planned about having the Joomla directories and files names camel cased, that's why I did the two options.

I think there are a few advantages using camel cased directories and files :
The directories and files names directly match the namespace/class names so the loading is faster.
Also, it leads to clearer names, when you have two parts in a directory/file name, it doesn't look so good when lower case.

Contributor

LouisLandry commented Oct 11, 2012

I wouldn't say we have plans one way or another, but we do plan to explore our options. I like the possibility of using it regardless of the path we take for the core platform libraries going forward.

Contributor

florianv commented Oct 11, 2012

Ok. Maybe 3 options are needed just in case :

1- Lower case

2- Mixed case

3- Camel case

In the future, if we want the best speed, we can use maps : an array of keys containing namespace names and values the full path to it, so the loading is direct.
And we could have a cli app to generate the map.

http://www.zyxist.com/en/archives/140

Contributor

florianv commented Oct 13, 2012

So, I went ahead with the three options and updated the setup method.
You will tell me if you agree with that.
I also added the $allowClasses param for the class map loader at the end because it might be first depreceated, once Joomla is fully auto loadable.

Then, I played around and tried various methods, and it seems that my first implementation was not the fastest one.
Moreover the explode/implode speed changes too much dependending on the length of the class name which is not good.

I also removed the cost of the class_exists in the namespace load functions and used include_once instead.

In a nutshell, it is three times faster on my machine.

I also decided to split the three loaders in three separated methods.
I know some people will tell this is duplicated code, but why affording the cost of some 'if' at each load when we are aware of the case strategy to use on setup.

I also complemented the test with an other class JLoaderNamespaceTest to test that it really works when the class names are passed to the auto load function by PHP. I thought they were a leading '' but it is not the case.

It also splitted the tests as @ianmacl said.

I fixed a few editor warnings in the other load methods. One @throws missed, and some other were declared void but returned a boolean.

Contributor

eddieajau commented Nov 9, 2012

@florianv would you mind taking a copy of this file:
https://github.com/LouisLandry/joomla-platform/blob/manual/docs/manual/en-US/chapters/introduction.md

and making a gist or similar for with with additional documentation explaining the new namespace support and how developers can use it. We can merge it later when we redo the docs.

Thanks in advance.

Contributor

florianv commented Nov 9, 2012

@eddieajau Do you want me to replace the actual loader.xml docbook by a markdown file, merge the additions and include it in the commit ? If I understand the docs will be translated to markdown files ? Thanks.

Contributor

eddieajau commented Nov 9, 2012

No, don't replace it. A gist will be fine for now.

Contributor

florianv commented Nov 10, 2012

Contributor

eddieajau commented Nov 11, 2012

Thanks. We've just converted all the docs over to markdown so just update
your branch and make the change to the appropriate file.

Contributor

florianv commented Nov 11, 2012

I have rearranged a few things in the doc

Contributor

LouisLandry commented Nov 11, 2012

This is great @florianv. I'm going to give you one last nit-picky thing then I promise I'm going to merge it. Would you please add JLoader class constants for your JLoader::setup() $caseStrategy argument? It's JLoader::setup(JLoader::NATURAL_CASE) is more immediately understandable than JLoader::setup(2), even if the latter is more verbose.

Contributor

florianv commented Nov 11, 2012

Thanks. Yes indeed it is clearer. I don't squash the commits so you don't have to read the whole diff.

Contributor

LouisLandry commented Nov 12, 2012

Awesome @florianv, you are the very model of a fantastic contributor... thank you!

LouisLandry added a commit that referenced this pull request Nov 12, 2012

@LouisLandry LouisLandry merged commit 187bef3 into joomla:staging Nov 12, 2012

Contributor

dongilbert commented Nov 13, 2012

Awe yeah!

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