kohana / kohana

Basic application with official modules included
http://kohanaframework.org
Other
1.54k stars 427 forks source link

Move module loading functionality from core into its own modern package #34

Open lenton opened 10 years ago

lenton commented 10 years ago

I've taken all of the module loading functionality out of kohana/core and placed it into its own modern package: https://github.com/lenton/modules. I propose that this repository gets transferred to kohana/modules and then implemented into Kohana 3.4.

Firstly to clear up any confusion, kohana/core is just a module. The functionality for loading a module shouldn't be inside of a module which is why it's important for it to be separated into its own non-module package. The benefits of having a separated module loader is that whenever you just need Kohana module loading/support, you can simply require this package instead of the very large kohana/core. It solves the problem of the module loader being a module itself; this is why in bootstrap we currently have to manually include Kohana_Core before loading the modules, whereas this package can simply be loaded using the composer autoloader. Also, because this package is very modern (composer, phpspec, PSR-2, SOLID, DRY) it's another step towards modernisation for the framework.

Current Bootstrap

// Load the core Kohana class
require SYSPATH.'classes/Kohana/Core'.EXT;

if (is_file(APPPATH.'classes/Kohana'.EXT))
{
    // Application extends the core
    require APPPATH.'classes/Kohana'.EXT;
}
else
{
    // Load empty core extension
    require SYSPATH.'classes/Kohana'.EXT;
}

...

// Enable kohana module autoloader
spl_autoload_register(array('Kohana', 'auto_load'));

// Enable old kohana module autoloader
//spl_autoload_register(array('Kohana', 'auto_load_lowercase'));

...

// Enable modules
Kohana::modules([
    'application' => APPPATH,                         // Main application module
    //'auth'        => $vendor_path.'kohana/auth',      // Basic authentication
    //'cache'       => $vendor_path.'kohana/cache',     // Caching with multiple backends
    //'codebench'   => $vendor_path.'kohana/codebench', // Benchmarking tool
    //'database'    => $vendor_path.'kohana/database',  // Database access
    //'image'       => $vendor_path.'kohana/image',     // Image manipulation
    //'minion'      => $vendor_path.'kohana/minion',    // CLI Tasks
    //'orm'         => $vendor_path.'kohana/orm',       // Object Relationship Mapping
    //'unittest'    => $vendor_path.'kohana/unittest',  // Unit testing
    //'userguide'   => $vendor_path.'kohana/userguide', // User guide and API documentation
    'core'        => SYSPATH,                         // Core system
]);

...

// Initialize modules
Kohana::init_modules();

...

Bootstrap after implementation

use Doctrine\Common\Cache\FilesystemCache;
use Kohana\Modules\Filesystem\CascadingFilesystem;
use Kohana\Modules\Autoloader\ModulesAutoloader;
use Kohana\Modules\Autoloader\LegacyModulesAutoloader;
use Kohana\Modules\Initializer\ModulesInitializer;

// Enable composer autoloader
require $vendor_path.'autoload.php';

// Instantiate cascading filesystem
$cache = new FilesystemCache(APPPATH.'cache');
$cfs = new CascadingFilesystem($cache, [
    SYSPATH,                         // Core system
    //$vendor_path.'kohana/cache',     // Caching with multiple backends
    //$vendor_path.'kohana/userguide', // User guide and API documentation
    //$vendor_path.'kohana/auth',      // Basic authentication
    //$vendor_path.'kohana/codebench', // Benchmarking tool
    //$vendor_path.'kohana/database',  // Database access
    //$vendor_path.'kohana/image',     // Image manipulation
    //$vendor_path.'kohana/minion',    // CLI Tasks
    //$vendor_path.'kohana/orm',       // Object Relationship Mapping
    //$vendor_path.'kohana/unittest',  // Unit testing
    APPPATH,                         // Main application module
]);

// Enable Kohana module autoloader
spl_autoload_register([new ModulesAutoloader($cfs), 'autoload']);

// Enable legacy Kohana module autoloader
//spl_autoload_register([new LegacyModulesAutoloader($cfs), 'autoload']);

...

// Initialize all modules
(new ModulesInitializer($cfs))->initialize();

Maintaining backwards-compatibility when implementing it in kohana/core will prove difficult. The cascading filesystem object could be set as Kohana::$cfs. This way the CFS would still be accessible from everywhere and some of the existing CFS related methods could become aliases and still work. These methods that haven't been removed would however become deprecated as it'd be recommended to use the CFS object instead.

samwilson commented 10 years ago

This looks like a good idea.

rakucmr commented 10 years ago

+1

rafi commented 10 years ago

Good direction. There is a great danger with a large number of modules and config/i18n/messages files when merged, it scans every single directory to accomplish the merge effect of these special type of files. In high-load traffic apps make sure Kohana::$caching==TRUE and override Kohana::cache() to use a cache driver, just like the $log and $config pattern. File-system caching is miserable.

Note that in development obviously you want Kohana::$caching==FALSE, however this means that every call to find_file() the file-system is being traversed again and again in search for files, on every call. Again, this is miserable, specially within virtual-machine environments.

For these reasons I reduced my usage of find_file() strictly to media files.

rjd22 commented 10 years ago

This is actually a really good direction to go in. This way it will be easier in the future to separate the core into small self contained modules.

This step may also enable use to load modules of other frameworks into our application without having to add support to our core.

lenton commented 10 years ago

It's probably best if we make this module manager a proper component first instead of releasing it as a Kohana module. At some point in the future I will begin recoding it using the Ohanzee standards such as phpspec, psr, composer, SOLID, etc. It would still be a good idea to transfer it to kohana/modules now though so everyone can contribute to it.

rjd22 commented 10 years ago

@shadowhand could you pull it in into kohana/modules ?

shadowhand commented 10 years ago

@rjd22 lost this deep in my inbox... are you still waiting for me to do something with this, or did @enov take care of it already?

enov commented 10 years ago

Maybe it's too early for this, probably in 4.0?

lenton commented 9 years ago

Maybe it's too early for this, probably in 4.0?

Have we considered making the next release 4.0 instead of 3.4? I feel we are trying to make big changes to set Kohana up for modernisation but are limited because we have to maintain BC for the minor release. As @acoulton has said; 3.4 is already going to be difficult for existing applications to upgrade to because of the major structural changes compared with previous minor releases. I think we need the freedom of a major release.

enov commented 9 years ago

3.4 is a major release @lenton. We only maintain BC wherever possible. But the whole thing should be upgradable, as opposed to writing a new framework as version 4.0 might suggest.

I am :+1: for this, as well as for multiple modules, instead of something called core. I am also for dropping modules like ORM, Auth, codebench, maybe even image and database at some point in the future.

enov commented 9 years ago

But the main issue is what are the priorities now, what to do next, and how far we're going to push maintaining the framework? I think these are the most discussed issues right now (trying to make it the smaller list possible):

I think we all agree about everything in this small list, but I find we disagree where to start and how far we need to push maintaining the framework.

enov commented 9 years ago

@shadowhand could we have @lenton in the team?

lenton commented 9 years ago

3.4 is a major release @lenton. We only maintain BC wherever possible. But the whole thing should be upgradable, as opposed to writing a new framework as version 4.0 might suggest.

3.4 is a minor release: http://semver.org. We are wanting to make huge changes in 3.4 that mostly can't be made BC. We would just be abusing the semantic versioning if we pass 3.4 of as mostly BC and easily upgradeable.

Shall I create an issue to vote on this?

enov commented 9 years ago

We do not follow semantic versioning properly.

enov commented 9 years ago

In terms of Kohana versioning, 3.4 is a major release.

Shall I create an issue to vote on this?

To follow semantic versioning instead of the current versioning system? Sure, go ahead.

shadowhand commented 9 years ago

@shadowhand could we have @lenton in the team?

@enov Invite to Community team has been sent to @lenton. :)

@lenton please continue to use pull requests for any changes related to the module manager, this needs to be carefully code reviewed to make sure it properly meets the requirements of code separation and extensibility.

lenton commented 9 years ago

@shadowhand Thanks! And absolutely, I won't be recklessly making important decisions without the review and consensus of the community first.

lenton commented 9 years ago

I've finally finished coding this package! It's now a modern composer package which strictly follows the PSR-2 coding style and has tests written in phpspec. All the coding is complete, I'm just currently moving over and rewriting the documentation related to modules and the CFS from the core into its readme.

I've updated my first post with the new implementation example. If the reviews are all positive then hopefully the repository can be moved over to the Kohana organisation.

shadowhand commented 9 years ago

@lenton I haven't looked closely at the code, but the API looks good!

lenton commented 9 years ago

When anyone has some time could they please review this package, I'm very eager to get it implemented into 3.4.

lenton commented 9 years ago

@enov @acoulton @rjd22 ^

rjd22 commented 9 years ago

@lenton looks good to me. Is it PSR2 checked with phpcs? If so you have my ok.

enov commented 9 years ago

@lenton, it will take some time to have a comprehensive look, please allow me another 24hrs.

enov commented 9 years ago

@lenton, I think this is great and this is the way that Kohana should take.

Please note the following:

I think these are my main concerns, thank you for your patience.

Otherwise, it is a great job. Thank you for your hard work.

samwilson commented 9 years ago

On the topic of filesystem interfaces, there's a pretty good Kohana module: https://github.com/morgan/kohana-storage I've used it a bit, and enjoyed it.

Or, in addition to the three listed by @enov, there's https://github.com/KnpLabs/Gaufrette (which I've not used, but seems to be quite comprehensive).

acoulton commented 9 years ago

@lenton thanks very much for this and sorry it's taken so long to find time to have a proper look.

How much of a detailed review are you looking for? For example, when I looked at the Modules autoloader I noticed that there's a fair bit of class name => file name mapping that's not covered by your specs, and that there's some duplication between the Modules and ModulesLowercase autoloader that could probably be refactored. Similarly in the CFS object, there's some duplication between getPath and getAllPaths for example. Do you want that sort of detail yet?

I noticed that the CFS load method uses include - shouldn't the autoloader and initialiser be using require_once?

On @enov's points:

One final thought - might it be better to allow the user to specify PSR-0 or legacy (lowercase) file naming on a per-module basis? I don't think there'll be many people using this new package who need to have the whole system in lowercase, but if you could specify that a particular directory should be mapped as lowercase that would make it much easier to use 3.4 with older application or third-party code. Not sure...

lenton commented 9 years ago

@enov

Also the main Kohana\Modules\Autoloader\Modules::load should be preferably renamed to autoload.

Sure, I suppose technically an autoloader should autoload.

Cache interface: if we're going all the way PSR-1/PSR-2, I would prefer we go for the FIG's solution also for this case, and use their proposed cache interface. It will help the project be future proof and you can plug any available cache solution later on.

Cool, didn't know PSR had a cache interface, I'll have a go at implementing that.

This way, someone could have a CascadingFilesystem only for the resources and not for the classes.

The CascadingFilesystem is already completely agnostic of the extra Kohana module abilities such as initialisation and autoloading. To not have classes you simply don't register the autoloader.

Previous work done: Kindly take also a note of @zombor's refactor of Kohana core in case you did not still have a look at it.

Yep, I was using it as a reference, the package is very similar to @zombor's implementation.

Various nomenclature issues: kohana-modules is so ambiguous. Maybe "Loader"? Or any generic, latin, catchy name could go there, you get the point.

This package's sole purpose is to add support for Kohana modules to an application, so I think "modules" is the perfect name really. "Loader" is even more generic.

@acoulton

How much of a detailed review are you looking for? For example, when I looked at the Modules autoloader I noticed that there's a fair bit of class name => file name mapping that's not covered by your specs, and that there's some duplication between the Modules and ModulesLowercase autoloader that could probably be refactored. Similarly in the CFS object, there's some duplication between getPath and getAllPaths for example. Do you want that sort of detail yet?

I'll take a look into adding those specs and refactoring to remove the duplication on the methods you noted.

I noticed that the CFS load method uses include - shouldn't the autoloader and initialiser be using require_once?

I agree.

Cache interface - I don't think the cache implementation should be part of this package, and I agree we should use a PSR cache interface if we can.

If this package doesn't provide a basic cache implementation, what are we going to use in kohana/kohana?

Also don't we need a way to disable caching in development?

What do you suggest, passing null instead of the cache object?

Runtime module changes - this is definitely required, and need a way to init any added modules without re-initialising the others.

Ok.

Naming - I think it's better to avoid Interface in the interface name, and to be more explicit in the class name so it's clear what it does when you don't see the namespace . For example, I'd have interface Kohana\Modules\Autoloader\Autoloader and class Kohana\Modules\Autoloader\ModulesAutoloader rather than AutoloaderInterface and Modules.

Ok.

One final thought - might it be better to allow the user to specify PSR-0 or legacy (lowercase) file naming on a per-module basis? I don't think there'll be many people using this new package who need to have the whole system in lowercase, but if you could specify that a particular directory should be mapped as lowercase that would make it much easier to use 3.4 with older application or third-party code. Not sure...

I'll look into a solution.

Thanks for the feedback! I'll get to work on your suggestions.

enov commented 9 years ago

@acoulton, @lenton

Cache interface - I don't think the cache implementation should be part of this package

I tend to agree with this as I have a feeling that it is better not to cache at this point, as you said, it is tricky especially when developing.

kohana autoloader would then have to be registered with the prepend flag

I wasn't aware that spl_autoload_register had a prepend flag. That's a cool solution.

Filesystem interface - do we really need that right now?

If we are not going to be based on one of the available options, we do need to have one defined in the package. It makes things extendable and the autoloader class should rely on that interface.

enov commented 9 years ago

@lenton

I agree that this is to make the application load modules, but I am afraid that calling this kohana-modules sounds, at least to me, a repository/list of Kohana modules.

acoulton commented 9 years ago

@lenton

Cache interface - I don't think the cache implementation should be part of this package, and I agree we should use a PSR cache interface if we can.

If this package doesn't provide a basic cache implementation, what are we going to use in kohana/kohana?

Could we maybe use doctrine/cache? It's not PSR (that interface isn't stable, I see, and might become too heavyweight anyway for what we want). I think the doctrine interface would work for us, it's well maintained, doesn't have any dependencies, and it has all the drivers we'd need. Plus it's so widely used in Doctrine-based projects that if/when the PSR lands I bet someone will make a PSR-to-doctrine-cache bridge package to allow other PSR-compatible cache drivers to satisfy their interface (or we could).

We could then in future consider making kohana/cache more of a thin config/initialisation/instance management package around doctrine/cache as well, rather than maintaining all our drivers separately.

Also don't we need a way to disable caching in development?

What do you suggest, passing null instead of the cache object?

doctrine/cache provides an ArrayCache implementation that has no state between requests - something like that would work. Then the autoloader/CFS has no knowledge of whether or not it's caching, and you may get a small benefit from caching some path lookups for the duration of a singl request.

@enov

Filesystem interface - do we really need that right now?

If we are not going to be based on one of the available options, we do need to have one defined in the package. It makes things extendable and the autoloader class should rely on that interface.

Why? The only thing our autoloader does different to any other PSR autoloader is to load from the CFS. If you want to load classes from some other part of the filesystem, just register a composer autoloader or whatever, before or after ours depending on your usecase.

I agree that this is to make the application load modules, but I am afraid that calling this kohana-modules sounds, at least to me, a repository/list of Kohana modules.

I agree, kohana-modules feels wrong, so does kohana-loader. What about kohana-module-manager, kohana-module-loader? I can't think of any single-word name that's clearer.

enov commented 9 years ago

Why? The only thing our autoloader does different to any other PSR autoloader is to load from the CFS

Yes, but it does not hurt to have an interface defined. Eventually, this filesystem object should replace all Kohana::find_file calls (even for other resources like views and configs...) and since this same object is used by the autoloader to load the classes, it might at some point need to be extended.

<ramble> Further down the road, we might want also define an interface for module objects. Instead of calling a module's init.php file, the manager-loader (which should also hold those objects) creates objects and passes the filesystem (or maybe itself?) to the constructor, which initialize the module with the filesystem instead of the globals we are trying to remove. That would be augmenting too much this manager's responsibilities (a DI container?). <⁄ramble>

Someone once said: think of the children.

acoulton commented 9 years ago

OK, I was confused by references to existing filesystem interface projects.

No objection to having this package provide a simple interface and a CascadingFileSystem class implementing it, but I don't think there's any need to make that a standard interface. Just extracting the method signatures as-is from CascadingFileSystem would do me.

I'm assuming that the CascadingFileSystem object will replace the Kohana::find_file etc calls as part of this new package (rather than 'eventually') since this will completely replace existing module definition and initialisation...

shadowhand commented 9 years ago

Based on a quick review of all the classes, I see a number of style issues:

and some implementation issues:

If you would prefer, I can make actual Github issues from these comments.

lenton commented 9 years ago

there is no consistency with braces, both newline and non-newline braces are used

PSR-2 dictates that class, method, and function braces must be on a new line whereas "control structure" braces must be on the same line.

only redundant comments used in some places where no comments are necessary

Is there really such thing as too many comments?? :P

several classes have properties that can only be overloaded by extension, instead of having configuration options via constructor or setters

I assume you're referring to the Initializer and Autoloader. These properties were purposefully made non-changeable to the user because they are a standard that needs to be upheld by all modules for compatibility reasons. If the user wants to use their own standard they must create a different class which uses the initializer/autoloader interface.

file paths sometimes hard code / and sometimes use DIRECTORY_SEPARATOR

Should I hard code the rest of them?

The rest of your feedback has been placed on my todo list, thanks @shadowhand.

shadowhand commented 9 years ago

PSR-2 dictates that class, method, and function braces must be on a new line whereas "control structure" braces must be on the same line.

Correct, but you have several places where control structures (specifically foreach) have newlines and should not. Try running PHPCS against your repository.

Is there really such thing as too many comments?

Yes, there is. Specifically:

Don't write what code is doing

I see a lot of this in your repo and it just creates noise. Comments should only be used when it is necessary to explain things, and for docblocks.

Should I hard code the rest of them?

Choose one or the other, just don't do both. :)

zombor commented 9 years ago

Is there really such thing as too many comments?? :P

The only comments that should exist are docblocks. Comments that explain what code does means the code is too complicated and should be rewritten.

lenton commented 9 years ago

I've made the following updates to the package:

I believe that's everything from the feedback except 2 things.

I deemed it pointless implementing a filesystem interface at this stage because the cascading filesystem's API is currently too far from that of a general filesystem.

I also haven't added the feature of being able to add or remove modules at runtime as I'm sceptical that this is actually good practise or even needed. I think the list of available modules should remain static just like the composer package autoloader.

I've updated the bootstrap implementation preview in my first post with the updated API. If I haven't missed anything out, I think now's a good time to transfer the repository over to the Kohana organisation.

shadowhand commented 9 years ago

Removed 'Interface' from all interface names.

I don't agree with that change at all... pretty much every framework out there includes the Interface suffix to help developers quickly find known interfaces.

lenton commented 9 years ago

@shadowhand I wasn't sure on this one either. I think it might be better to be explicit, and it makes sense since I already use the Abstract prefix. PSR-2 even uses this naming style in their code examples.

I'm willing to revert this unless @acoulton wants to defend it?

rjd22 commented 9 years ago

You should use the interface suffix. It makes it clear that you're type hinting with an interface without viewing the class.

Abstract are different because they are objects that need to be extended.

enov commented 9 years ago

I deemed it pointless implementing a filesystem interface at this stage because the cascading filesystem's API is currently too far from that of a general filesystem.

It's the only way for NOT to dictate our implementation on users, as in itself, the CascadingFilesystem is not extendible even through the CFS it tries to implement. It is better to provide a shoddy interface for now, as opposed to not to have at all.

PSR-2 even uses this naming style in their code examples.

the FIG has an additional bylaw for code released by itself: https://github.com/php-fig/fig-standards/blob/master/bylaws/002-psr-naming-conventions.md

acoulton commented 9 years ago

I follow Uncle Bob's logic on the Interface suffix. It:

I'm in a rush, so not sure I've done a good job of presenting his argument. He does a good job of putting his case in 'clean code' - good enough to make me change my practice, anyway :)

to help developers quickly find known interfaces

Most IDEs will satisfy this whatever they're called, or without an IDE it's surely just a question of grep ' interface \w+' instead of grep '\w+Interface'.

It makes it clear that you're type hinting with an interface

What benefit does this provide? The type hint doesn't specify the actual class the method receives (because it might get a subclass, for example) so it's no help when tracing back for debugging or understanding. If it's so you can work out how to customise the collaborator it receives then you will need to look at the collaborator implementation anyway, at which point you'll easily see whether it's an interface or a concrete type.

I get that it's a convention, so was sName and bIsActive. if you want to stick with it then I won't object, but I definitely feel like dropping it has made a positive improvement to the clarity and isolation of my code.

shadowhand commented 9 years ago

@acoulton reasonable arguments, I still think it could go either way.

lenton commented 9 years ago

I guess the only way to resolve the interface naming dispute is to have a vote: I vote for appending 'Interface'.

It's the only way for NOT to dictate our implementation on users, as in itself, the CascadingFilesystem is not extendible even through the CFS it tries to implement. It is better to provide a shoddy interface for now, as opposed to not to have at all.

ATM the class can be extended and methods overridden if the user wants their own implementation. However we may well end up implementing a filesystem interface before the 1.0 release.

@shadowhand How would I go about getting the repository transferred to Kohana?

shadowhand commented 9 years ago

@lenton the easiest way to transfer ownership to Kohana is for you to request transfer of the repository to Kohana organization from the repo settings. I've created a team for you that gives you the admin permissions necessary to do this (and add other team members if you want).

edited

shadowhand commented 9 years ago

@lenton check that last comment, just edited it.