Skip to content
This repository was archived by the owner on Nov 26, 2017. It is now read-only.

Namespaces support for JLoader#1591

Merged
LouisLandry merged 3 commits intojoomla:stagingfrom
florianv:jloader-namespaces
Nov 12, 2012
Merged

Namespaces support for JLoader#1591
LouisLandry merged 3 commits intojoomla:stagingfrom
florianv:jloader-namespaces

Conversation

@florianv
Copy link
Copy Markdown
Contributor

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).

@LouisLandry
Copy link
Copy Markdown
Contributor

@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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@florianv
Copy link
Copy Markdown
Contributor Author

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.

@florianv
Copy link
Copy Markdown
Contributor Author

@ianmacl Thanks, I will split them asap.

@LouisLandry
Copy link
Copy Markdown
Contributor

@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?

@florianv
Copy link
Copy Markdown
Contributor Author

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.

@LouisLandry
Copy link
Copy Markdown
Contributor

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.

@florianv
Copy link
Copy Markdown
Contributor Author

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

@florianv
Copy link
Copy Markdown
Contributor Author

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.

@eddieajau
Copy link
Copy Markdown
Contributor

@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.

@florianv
Copy link
Copy Markdown
Contributor Author

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.

@eddieajau
Copy link
Copy Markdown
Contributor

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

@florianv
Copy link
Copy Markdown
Contributor Author

Here is the gist : https://gist.github.com/fa4ea8ffc7b3b34151b2

@eddieajau
Copy link
Copy Markdown
Contributor

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

@eddieajau
Copy link
Copy Markdown
Contributor

@florianv
Copy link
Copy Markdown
Contributor Author

I have rearranged a few things in the doc

@LouisLandry
Copy link
Copy Markdown
Contributor

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.

@florianv
Copy link
Copy Markdown
Contributor Author

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

@LouisLandry
Copy link
Copy Markdown
Contributor

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
@dongilbert
Copy link
Copy Markdown
Contributor

Awe yeah!

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants