Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Namespaces for controllers modules and helpers #571

Open
rjd22 opened this issue Nov 19, 2014 · 82 comments
Open

Namespaces for controllers modules and helpers #571

rjd22 opened this issue Nov 19, 2014 · 82 comments
Labels
Milestone

Comments

@rjd22
Copy link

rjd22 commented Nov 19, 2014

We should start thinking about implementing namespaces in Kohana and it's modules but also decide on a vendor / prefix.

Like \Kohana\<module>\<class>

Also enable users to namespace their own application. And point the routes to the namespace.

What do you guy's think? (Typed this on my mobile so keeping it short)

@rjd22 rjd22 added the Idea label Nov 19, 2014
@rjd22
Copy link
Author

rjd22 commented Nov 20, 2014

@enov @lenton @acoulton @shadowhand pinging you for some opinions :)

@kemo
Copy link
Member

kemo commented Nov 20, 2014

There was discussion on this topic before;
http://forum.kohanaframework.org/discussion/9740/kohana-and-namespaces/p1

Anyway, this definitely gets a 👍 from me - right after we make some things clear;

  • Which version should we aim this for (maybe this should be Kohana4)?
  • Should we convert to PSR-1/2 at the same time?
  • Therefore, should we use Ohanzee components where possible / strip the framework completely
  • Would CFS / transparent extensions stay?

@enov
Copy link
Contributor

enov commented Nov 20, 2014

I am all for using namespaces @rjd22 😄

But the question remains: at what stage?

Limiting the scope of features for 3.4 would help us be able to release it at some point.

A namespaced version of Kohana could go out as a separate version on it own. I mean no new features in that release, except that it is a new Kohana based on the older version, but namespaced.

Another option would be to namespace while we work on some classes. But in that case we would have partly namespaced and partly non-namespaced classes. We can give preferences to the classes that are internally instantiated first. I mean, usually, a user does not do a new Log() as it is done internally somewhere in Kohana::init. That way, the new version is upgradeable by the user. Upgradeable releases is something I care about, as opposed to all new full-rewrite releases.

Keep in mind we always support 2 dev versions. Refactoring those class names means refactoring everywhere those are instantiated. It means going farther from the previous version. It means more merge conflicts when we try to merge up bug fixes from below.

Also enable users to namespace their own application

Users can already namespace their classes in 3.3 as it is PSR-0 compliant - except for the Controller classes, I guess.

@enov
Copy link
Contributor

enov commented Nov 20, 2014

Also enable users to namespace their own application. And point the routes to the namespace.

Yeah, I see now that you are concerned about the Controllers to be able to be namespaced.

@acoulton
Copy link
Member

I think this would be really good - for starters it would make working with tools like PHPSpec, and integrating with other components and projects, much easier.

It does feel like a big change to get in for 3.4, and I agree with @enov that we should avoid 3.4 becoming the "fix absolutely everything to the point we never actually release it" version.

Equally, I think there's quite a bit we could do to make the upgrade be relatively painless:

Auto-upgrade script

It would be fairly simple to build a small script that finds all references to official Kohana classes (scoped to only consider modules active in the project, so that it doesn't conflict with some third-party "Auth" class for eg if the user isn't using Kohana Auth.

It could then just add appropriate use statements to the top of the relevant files, so the rest of the file remains unchanged. We could use that script on the kohana packages themselves, and users could also use it on their own applications.

This would mean the only major diffs between versions would be the use blocks themselves in the file headers - so there should be fairly limited potential for merge conflicts.

There's no real reason the class files themselves have to move, but even if they do so long as that's done properly with git it will follow the rename/move when merging up in future and apply the patch to the new path without conflicts.

Legacy shim

We could easily provide a separate package that just defines all the old non-namespaced classes, either as aliases or as class definitions. So end-user code could still refer to \Log but get an instance of \Kohana\Core\Log - not sure whether alias or extension is better, it possibly depends on how aliases behave with instanceof.

Potential related changes

Moving to ohanzee (or any other third-party) components

I think this is too big to do it at the same time - but properly namespacing would help with this in future and we should be moving in that direction.

Dropping the CFS and transparent extension

+1 from me. I like the CFS for things like config files, view templates, messages and stuff (though there would be other, possibly more explicit, ways to achieve the same thing). I used to love transparent extension, but now I think that properly mapping the classes in use in the project makes more sense.

IMO the best solution is to drop transparent extension altogether, and have end-users and module developers just depend directly on the (namespaced) interfaces and/or class implementations they actually need. That would also allow us to drop our autoloader and use composer directly which would simplify quite a lot.

Again, I think it would be easy enough to build a tool to help with this migration - look through the application and modules for customised transparent extension classes, add a custom namespace to the top of the file and then add a use statement to anything that uses it. The only issue would be where people are using transparent extension to customise behaviour of methods called by other kohana/core classes - where you wouldn't be able to change the imports.

One idea proposed in the past was virtual namespacing (using class aliases, I think) where Kohana/Log wouldn't actually exist and the system would automatically map it to Application/Custom/Log or Kohana/Core/Log as required. That might be an OK transition plan, while we work on making it easier to inject implementations into the core rather than relying on monkeypatching the classes the core is hardcoded to use.

It feels to me like that would be better done with some sort of development-time compilation step rather than a magical run-time autoloader defining aliases on the fly. So all the empty extensions would be removed from the core packages, in favour of a script that developers run to spit out appropriate autoloadable class definitions / class aliases somewhere in their /application directory. Any time you add a module or application class that "transparently" extends core you explicitly re-run the script and probably commit the result. That way both developers and IDEs are always clear on actually what class is being used. Note that you'd run this on the developer machine, it doesn't require any shell access to "compile" on the server.

Move to PSR1/PSR2

I fairly dislike quite a bit of these, though I'm gradually accepting that consistency is more important than preference.

But trying to migrate the existing code wholesale will create huge diffs and make merge conflicts between versions almost guaranteed. We could adopt PSR1/PSR2 for entirely new files, but I think we should stick with what we have for the existing stuff - otherwise maintenance is going to become a nightmare.

@acoulton
Copy link
Member

Sorry, long post - should have followed @rjd22 example and posted on my mobile! Also to say of course there's no reason we can't make routes support namespaces separately from actually namespacing the core - that could be a 3.4 feature without any major issue I think?

@acoulton
Copy link
Member

@cyrgit PSR4 - absolutely.

Coding style - only if you're volunteering to be in charge of merging up bugfixes from the 3.x series to whatever version we introduce all the changes. Every commit will have merge conflicts over casing and whitespace to the point it'll potentially be easier to rewrite patches by hand.

Unless we change the release policy so that only one version is supported at a time, and bugfixes never get merged up between versions but IMHO that would be a much worse outcome for users than having a mix of conventions (frustrating though that is).

Ohanzee can do it because it's starting from scratch.

@lenton
Copy link
Contributor

lenton commented Nov 20, 2014

I say we split the core into separate kohana modules in 3.4, then in 4.0 we can convert them to fully namespaced composer packages or switch them out with their ohanzee counterpart.

@shadowhand
Copy link
Contributor

Ohanzee has already moved in that direction (PSR4/camel case), why leave Kohana behind?

It sounds like you are treating Ohanzee as a separate project, one which is totally alien to Kohana. This is not the case. Ohanzee is part of the Kohana Foundation and anyone from Kohana is welcome to participate in Ohanzee.

This entire discussion seems to be largely an echo of what I was talking about ~8 months ago. Kohana is a legacy framework because it failed to make the jump to namespaces and PSR standards. Ohanzee was intended to resolve that in a clean way, by dropping the Kohana name and making a clean break on code style, offering a way to transition the best modules of Kohana to Composer packages.

IMO, Ohanzee is still the best way forward to transition Kohana to modern PHP code. Deprecate Kohana, say that the next 3.x release will be the last one, and put effort into porting the best of Kohana into Ohanzee. Attempting to modernize Kohana without putting effort into Ohanzee just ensures that Ohanzee will never gain traction. Just my $0.02.

@lenton
Copy link
Contributor

lenton commented Nov 20, 2014

I think we all agree that the ohanzee components are the way forward for Kohana, the updates we are making to kohana are really just a slow transition to using them.

I'm going to start work again on my "kohana module manager" package, I feel this will really be key for the transition. It would allow kohana to use a mixture of both proper components and kohana modules.

@rjd22
Copy link
Author

rjd22 commented Nov 20, 2014

(From mobile again)

I think we should just make a empty 4.0 branch and start building only the core components there with namespace support.

So only the bootstrapping capabilities first and focus on working together with other libraries.

There are so many libraries that solve the issues really well like Doctrine for DB.

I think it's okay to have our own libraries but we shouldn't forget the dame problem has been solved many times so we won't have to rewrite the whole framework right away.

@rjd22
Copy link
Author

rjd22 commented Nov 22, 2014

@lenton @shadowhand @cyrgit @acoulton @enov @kemo

Let's do a clear vote on the following questions. Please keep it short:

  • What Version should implement it?
  • Are we implementing PSR 1 and 2?
  • Will we keep transparent extension?
  • What modules / functionality should be in the first version?

@rjd22
Copy link
Author

rjd22 commented Nov 22, 2014

  • what version?
    4.0, we should make this non backwards compatible and won't back port funtionality
  • psr?
    Yes, it's becomming the defacto standard
  • Transparent extension?
    No, you should just put the extensions in your own namespace. We should check what parts might need extension and make it possible to register a extension with a con fig file.
  • modules / funtionality?
    Core, Request, Response, Session, Cookie, Controller, Security, Routing, Log, Profiler.

@Girt
Copy link

Girt commented Nov 22, 2014

I think we should keep an transparent extension. Otherwise, the philosophy of the kohana will change.

@enov
Copy link
Contributor

enov commented Nov 22, 2014

  • What Version should implement it?
    4.0
  • Are we implementing PSR 1 and 2?
    Yes
  • Will we keep transparent extension?
    No, drop CFS 😢 to embrace proper DI pattern
  • What modules / functionality should be in the first version?
    Sorry I can not make this short @rjd22 😉

@ursoforte
Copy link

*What Version should implement it?
4.0
*Are we implementing PSR 1 and 2?
Yes
*Will we keep transparent extension?
No
*What modules / functionality should be in the first version?
Core, Request, Response, Session, Cookie, Controller, Routing, Log (That seems to be essential for most projects start.)

@Ikke
Copy link
Contributor

Ikke commented Nov 22, 2014

  • What Version should implement it?
    4.0
  • Are we implementing PSR 1 and 2?
    Yes
  • Will we keep transparent extension?
    Not necessarily
  • What modules / functionality should be in the first version?
    Functionality:
    Routing, http, helper methods, database (PDO or MySQLi)

@JackEllis
Copy link
Contributor

What Version should implement it?
4.0

Are we implementing PSR 1 and 2?
Yes

Will we keep transparent extension?
_

What modules / functionality should be in the first version?
Request (controllers / views etc.), routing and helper methods... Kohana as delivery only is very appealing to me personally...

@enov
Copy link
Contributor

enov commented Nov 24, 2014

  • What modules / functionality should be in the first version?

I had to elaborate on the question.

I see the need of 2 kinds of modules:

  • There should be generic packages. These packages should contain only interfaces, traits and classes, and can be used in other projects/frameworks.
  • There should be Kohana specific modules that wrap the generic packages to simplify their inclusion and setup in Kohana. These modules might include a config folder, as well as the other resources (i18, message, views, etc...).

I would like to see kohana/core split into several packages, similar to the suggestion of @lenton. Maybe we need to drop ORM, Auth, codebench as well as the unittest modules, if possible.

@shadowhand
Copy link
Contributor

  • What Version should implement it?

v4

  • Are we implementing PSR 1 and 2?

Yes! PSR-1, 2, and 4.

  • Will we keep transparent extension?

Namespaces make it unnecessary, so no.

  • What modules / functionality should be in the first version?

Easier to say what NOT to keep. Drop ORM, Auth, Unittest, and Codebench. Ideally, replace as many modules as possible with Composer packages and write thin wrappers around them.

But I heard a strong argument that Kohana was supposed to have an upgrade path and I'm not sure this satisfies that. In many ways, it would be better to build a thin layer around Kohana to allow it to run on top of (eg) Symfony.

@enov
Copy link
Contributor

enov commented Nov 24, 2014

@shadowhand (and maybe also @cyrgit)

Namespaces make it unnecessary, so no.

Is there a way namespacing can support CFS without classes transparently extending other classes?

@shadowhand
Copy link
Contributor

Is there a way namespacing can support CFS

No, but namespaces make it unnecessary:

namespace Acme\Database;

use Kohana\Database\Query as KohanaQuery;

class Query extends KohanaQuery { ... }

The implication here is that everything has to be properly DI too. But it would be... right? ;)

@lenton
Copy link
Contributor

lenton commented Nov 24, 2014

  • What Version should implement it?

4.0

  • Are we implementing PSR 1 and 2?

Yes

  • Will we keep transparent extension?

We can have a mixture of both kohana modules (which use the CFS), and modern packages (ohanzee). Supporting old kohana modules would make the transition to these modern standards considerably easier for users.

  • What modules / functionality should be in the first version?

If 4.0 has support for kohana modules then we could retain all of the functionality from 3.4.

@enov
Copy link
Contributor

enov commented Nov 24, 2014

@cyrgit

If it read an \App\ folder, and then a \Kohana\ folder, it would serve as a CFS-like loader. If it finds the needed class in \App\ namespace, then it requires and returns true, else it continues to Kohana base.

Yes, but in that case, the "extending" class that is found in the \App folder should include all the functionality (methods and properties) of the class that's in \Kohana folder. It can not just extend the class to change, say, a method.

@rakucmr
Copy link

rakucmr commented Nov 24, 2014

@shadowhand drop ORM, Auth in favour of what?

@shadowhand
Copy link
Contributor

@Raku Auth is horrible, absolutely pathetic, for security. The new password_hash function and the password_compat library eliminate the need for it. There are many better packages that are easy to integrate for those that want it "easier".

ORM... well, if you're using ORM, I don't have any recommendations. Use Doctrine, maybe? ORM is the worst possible thing you can do for architecture, and I've been saying that it should be dropped for years. A new major version (v4) finally gives the opportunity to clean out the crap code and we should take it.

(And FWIW, I wrote the original versions of both Auth and ORM. Mistakes were made.)

@rakucmr
Copy link

rakucmr commented Nov 24, 2014

@shadowhand for ORM, what you say about this project https://github.com/spadefoot/kohana-orm-leap

@shadowhand
Copy link
Contributor

@Raku ORM is bad for architecture, no matter who creates it. Kohana ORM, LEAP, Doctrine... they all have the exact same architectural problem: combining the data model with persistence is Doing It Wrong.

@zombor
Copy link
Contributor

zombor commented Nov 24, 2014

Unless it's kept in a repository layer abstracted away from the main application (like all db access should be anyway).

@zombor
Copy link
Contributor

zombor commented Nov 25, 2014

And they still work. If you aren't proposing 100% BC, what's the point? You'll have to change all your code anyway.

@lenton
Copy link
Contributor

lenton commented Nov 25, 2014

The only way any of this makes sense is if Kohana doesn't break BC.

+1 The whole point of continuing with Kohana instead of abandoning it and contributing to Ohanzee was to create a smooth transition path for users to upgrade.

wouldn't it be extremely hard to be backwards compatible and still implement name spacing and the improvements we want?

If 4.0 still supports Kohana modules then it won't be as bad, I'm working on that now.

@enov
Copy link
Contributor

enov commented Nov 25, 2014

I am for an upgrade path, @zombor. Yes, if I need to change all my code, what's the point..

@zombor
Copy link
Contributor

zombor commented Nov 25, 2014

You can't have a "modern" framework without breaking BC in a major way, imo.

@enov
Copy link
Contributor

enov commented Nov 25, 2014

I am not expecting a fully modern framework in 4.0, just step by step.

@acoulton
Copy link
Member

we delete pretty much all the code except the class names and method signatures and just proxy them all to better third-party libraries

So it's like using other libraries code, but with Kohana API... Is that even possible?

Possible, probably pointless in many cases. Meant it as an extreme example, but underlining that I think it's only worth updating Kohana to make it easier for people who have existing applications to support/gradually disentangle them.

There are two possible future users of Kohana:

  • Existing applications
  • New applications by people who know and love Kohana how it is now and don't want to change

None of those people are going to use 4.0 - no matter how good it is - if it's totally different because there are existing, better, supported, active projects with communities around them already. Anyone who thinks we'll suddenly attract new people that aren't in those two groups is chasing rainbows.

@acoulton
Copy link
Member

I am not expecting a fully modern framework in 4.0, just step by step.

I'm not expecting a modern framework at all, just one that doesn't get in the way of me writing/migrating to a modern application. For me that mostly just means:

  • it doesn't pollute the global namespace unless I ask it to
  • it lets me manage my dependencies and stub/mock stuff easily when I'm unit testing my own classes
  • it lets me use namespaces in my own code
  • within reason it lets me customise or replace what it's doing

And that's about it.

@zombor
Copy link
Contributor

zombor commented Nov 25, 2014

If you write your application abstracted away from any framework, you satisfy all these requirements right now. The framework is a detail.

@acoulton
Copy link
Member

@zombor I do, and I'm moving the older projects that way as I can.

But 3.3 does get in the way of that at times, not least the fact that controllers have to be in the global namespace as is all the kohana stuff...

@rjd22
Copy link
Author

rjd22 commented Nov 25, 2014

BC compatibility seems still important so let's aim for that.

I would like to hear options on how to achieve it while supporting namespaces, PSR and removing the transparent extension.

IMO: the most hard part will be implementing PSR 1 and 2

@enov
Copy link
Contributor

enov commented Nov 25, 2014

I think there is a tool for PSR-1 and 2: http://cs.sensiolabs.org/

@acoulton
Copy link
Member

Why bother with PSR-1 and 2? Don't we just risk introducing bugs that weren't there before? Do we seriously ask end-users to search and replace everywhere they call a snake_cased Kohana method just so we can rename them? And what about things like controllers - are we going to change the router to look for actionSomething instead of action_something and then force everyone to change all of that too?

I really don't see that there's any benefit to changing coding standard in the existing code if the focus of the project is BC and legacy support.

If we're going to ask people to change their code for 4.0, we should respect their time and only make them do it for things that make a functional difference eg namespaces.

@rjd22
Copy link
Author

rjd22 commented Nov 27, 2014

@acoulton I think you said that you were working on the namespace support for the kohana routing. I'm in need of this so I'm thinking of working on it myself. Are you already working on this? And how we're you planning to implement it?

@acoulton
Copy link
Member

@rjd22 sorry, no not so far. I think I just said I didn't think it would be hard. I think you'd just have to modify Kohana_Request to accept an extra namespace parameter from the route definition and store it as a request property same as eg directory. Then Kohana_Request_Client_Internal would need to be modified to compile the fully qualified class name.

I'd suggest that if a namespace is defined, we shouldn't add a Controller_ prefix - and possibly should ignore the directory too. Maybe Route::set should throw if you try to define both namespace and directory on the same route, to avoid harder-to-trace problems later.

So eg a route like:

Route::set('default', '(<controller>(/<action>(/<id>)))')
     ->defaults(array(
                     'namespace' => '\My\Application\Controller',
                     'controller' => 'welcome',
                     'action'     => 'index',                      
                ));

would map to \My\Application\Controller\Welcome.

For bonus points I guess we could define controller_prefix and controller_suffix as route params so that you can still use <controller> as a dynamic URL parameter but be able to for eg call your class \My\Application\Controller\WelcomeController or whatever suits you. I tend to end up with a route per controller anyway, so it doesn't much matter to me.

I don't see that adding any BC issues except for the rare possibility that someone is using namespace as an actual request parameter name. In that case they'd get a 404 from those URLs on first upgrade because their namespace parameter is unlikely to map to an existing class.

@rjd22
Copy link
Author

rjd22 commented Nov 27, 2014

@acoulton looks good to me. I will see if I can make time this weekend. This should be able to go into 3.4

@acoulton
Copy link
Member

@rjd22 great, thanks :)

@rakucmr
Copy link

rakucmr commented Nov 27, 2014

@rjd22 Maybe this can help you https://github.com/kohana-ns

@loonies
Copy link

loonies commented Nov 27, 2014

Why don't we just drop namespace part in the route definition? Define
a route and map it to a FQCN and action. This way you can use arbitrary
class as a controller. The only requirement would be that a class
extends base controller class where request/response are injected.

@rjd22
Copy link
Author

rjd22 commented Nov 28, 2014

@loonies for the sake of discussion can you give some code examples?

@acoulton
Copy link
Member

@rjd22 I presume @loonies means just:

\\ Maps to \My\Application\Controller\Welcome::action_index()
Route::set('welcome', '(welcome(/<action>(/<id>)))')
     ->defaults(array(
                     'controller' => '\My\Application\Controller\Welcome',
                     'action'     => 'index',                      
                ));

Issue with that is it would require a separate route for each controller, since you probably don't want an FQCN in your URLS. I mostly do that anyway, but I think a lot of users don't and it wouldn't be BC with existing routes.

A halfway house might be to say eg:

if (substr($params['controller'], 0, 1) === '\\')
  $controller = $params['controller'];
elseif ($params['namespace'])
  $controller = $params['namespace'].'\\'.$params['controller']; // could apply optional prefix/suffix here
elseif ($params['directory'])
  $controller = $params['directory'].'_Controller_'.$params['controller'];
else
  $controller = 'Controller_'.$params['controller'];

Which would be fully BC, but would support new code providing either FQCN or namespace + class name.

@loonies
Copy link

loonies commented Nov 30, 2014

Yes, that's what I meant.

@acoulton: Good point with using controller as a param issue.

At least I would like that we drop the "Controller_" prefix or have the
ability to use arbitrary class name as a controller.

@localheinz
Copy link

This thread is tl;dr, so:

It's 2015, there are a lot of people using PHP5.5 and even PHP5.6 in production already. Not moving on and leveraging features of the PHP version required in composer.json will eventually mean to be left behind by the community, which may or may not move on to more up-to-date frameworks.

So, totally 👍 for using actual instead of pseudo-namespaces.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests