bcit-ci / CodeIgniter

Open Source PHP Framework (originally from EllisLab)
https://codeigniter.com/
MIT License
18.27k stars 7.61k forks source link

Override Hook on Controller declaration? (ex.: adding Dependency Injection for Controllers) #5102

Closed adaliszk closed 7 years ago

adaliszk commented 7 years ago

I don't know if it was discussed before, but I miss a hook override to create the main controller class $CI = new $class(); and then override the method request call_user_func_array(array(&$CI, $method), $params); in the core/CodeIgniter.php It would be nice to override them if it's necesseary; for example adding a dependency injection to the constuctor or anything else. This should not take too much to implement, basicly it's the same as the display_override:

if ($EXT->call_hook('display_override') === FALSE)
{
    $OUT->_display();
}

Before I make a pull request from this I wanted to ask that it's can fit in the current version? (I mean it will be accepted and merged or it's waste of my time to bother to do a pull request)

narfbg commented 7 years ago

That'd pretty much be overriding the entire framework. Why on earth would you want to do that?

adaliszk commented 7 years ago

I don't agree that this would override the entire framework, it's just add ability to override the main class declaration and method calling, the framework still parsing the input, handling the database and etc, also why allow us to override the core classes with anything if we should not do that? 99% of the time we just add more functionality and not replacing everything to a complete new framework. The system does not handle dependency injection on constructor, in this version it maybe wont be by default too, but with the core classes I could and I did override the loader so it could do that with models, libraries but with controllers I do not have any option to add this functionality.

I would be happy if I could add the dependency injection on contructor to the system but it wont be merged because it's not a functionality of the current system like the namespaced models and libraries was. If we capable to override any core classes with our own then why we could not hook into the main controller declaration? Sure it can override the whole system if we want, but if we want to do the framework completley different why would I ask to implement a way to hook it instead of implementing the feature what I want to hook it anyway and using my own fork of the repository?

I just want to add a minor thing which could help a lot while we waiting for the CI4 :wink:

narfbg commented 7 years ago

I don't agree that this would override the entire framework, it's just add ability to override the main class declaration and method calling

The entire framework depends on CI_Controller::__construct(). This is not a minor thing.

Whatever it is that you're trying to do, there's certainly other ways to do it.

adaliszk commented 7 years ago

The CI_Controller::__construct() wont be change at all, the MyController::__construct() will be changed this way. The $CI = new $class(); is calling the route based controller not the core controller.

How will be changed the CI_Controller::__construct() in any way without override-ing that method? I cannot not use the CI_Controller because the whole framework is build around it: I can replace it (with core/MY_Controller), but I need to extend it because the get_instance() wont work otherweise. I cannot harm the CI_Controller::__construct() with this hook:

if ($EXT->call_hook('controller_override') === FALSE)
{
    $CI = new $class();
}

If I want to change it I have a way already by declaring MY_Controller and not extending the CI_Controller and do my own logic in the __construct().

I could silence the "Too few argument" error by adding the pre_controller and post_controller_constructor hooks and I could do my own Dependecy Injected class declaration in the last one, but the error will still happen, it wil be show in the log and it will still executed in runtime instead of just doing the actual Dependency Injected class decaration:

$CI_Contsruct = new ReflectionMethod( $class, '__construct' );
$Dependencies = $CI_Contsruct->getParameters();
$Parameters = [];

foreach($Dependencies as $dependency)
{
    $dependencyName = $dependency->getClass()->name;
    $dependencyName = str_replace('CI_', '', $dependencyName);

    $Parameters[] = load_class($dependencyName, 'core');
}

$CI = new $class( ...$Parameters );

It would be shame that depite we have tools to override the system we are not allow to override them because you assume that everybody will broke or replace the system with it. In this way why has the system hooks in the first place?

narfbg commented 7 years ago

You write a LOT of text while saying very little, and it's hard to understand.

adaliszk commented 7 years ago

Then in TL;DR version:

narfbg commented 7 years ago

The CI_Controller::__construct() wont affected by adding the hook to replace the Controller creation.

That's a contradiction - controller instantiation is what triggers the constructor - and you know it.

The hook will allow us to change the Router determinated controller __construct call but we need to inhereit at some point the CI_Controller because without it the whole framework is not working.

... and? Nobody is questioning this, and I don't get the point you're making with it.

If I want to harm the CI_Controller::__construct() then I can override it with core/My_Controller.php already

That doesn't mean anything. You're trying to counter my argument that everything depends on CI_Controller::__construct(), but you're missing the point ... yes, if you want to - you can break anything, but then it will be broken and thus useless to you.
That doesn't mean you should be able to break anything just because you want to - there's countless use cases for extending/overriding the CI_Controller class; you've still not shown a good use case for overriding the instantiation itself.

Also - MY_, not My_ ... unless you change the configuration.

We can still do the same override functionality without change-ing the code but that would generate unnecesseary log and process time.

I don't recommend that you do this, but if you already can and it does the job - how is it unnecessary, and why are you here at all?

Adding Dependency Injection without using the constructor is uneffective and should be avoided. This is a reason to having hooks in the system so we don't need to make wrapper solutions.

It's not "uneffective" - that means literally nothing.

To be clear - I'm not saying it's a good practice either, but that's our problem to solve in the future, not one for your custom hooks.

Adding DI functionality is not part of the current 3.x version (like namespaces, traits, interfaces and abstract classes), I accept that, I just ask a better hook point to integrate it and open a door for similar things.


Please stop arguing and just explain exactly what you want to achieve - problem solving starts from the problem itself ... you're asking for a solution to an unknown problem.

There may be better ways to do it than what you have thought of. There may even be better ways to achieve it that are already implemented. It may also turn out that you don't at all need to do whatever it is that you're trying to do.

adaliszk commented 7 years ago

That's a contradiction - controller instantiation is what triggers the constructor - and you know it.

And a controller __construct( $dependency ) is calling parent::__custruct( $dependency ) or it's calling the parent::__construct(); ? If nothing affect the the CI_Controller::__construct() then why we discussing about that?

I don't recommend that you do this, but if you already can and it does the job - how is it unnecessary, and why are you here at all?

If you have a spoon and a bucket to deliver water from one palce to an other you will use the spoon because eventually it's doing the same? I could hide the new $class(); and add my one with the current hooks but that would be a bad solution, because it would still generate the error message about mismatching paramethers on the class but after that I could at least do my custom one. It would be way easier if I could override it for my own needs.

That's our problem to solve in the future, not one for your custom hooks.

When? I waiting for the CI4 at least a year now and it's nowhere near to a preview version. Until I cannot use a fresh version I have to stick with the stable one but I need the new functionalites for creating better applications. I could use an other framework as well, but that's not helping at all.

You can easily add an autoloader, or pre-load traits and interfaces from inside a hook.

Yeah, so that's a job of my custom hooks but adding custom parts to the $CI = new $class(); is not fullify the "modify the inner workings of the framework without hacking the core files" condition so I shouldn't want that.

There's literally nothing stopping you from using abstract classes.

Try load an abstract class in the autoloader then, don't tell me that only way to use abstract class is to require them or load them by autoloader. This way I cannot load my app dependencies before I want them use it with the core system. (but I did solved the problem with an Autoloader)

HOW IS THAT RELATED AT ALL?!

Formerly I did maked a few pull requests to adding namespace and abstract class handling but I was strictly rejeted because that functionality is not part of the current version and I have to wait for the next version. So because I need that functionality I moved the code to the core classes and I override the system by that, that's what I accepted, that no new features will allow into the 3.x version.


Please stop arguing and just explain exactly what you want to achieve - problem solving starts from the problem itself ... you're asking for a solution to an unknown problem.

Okay so I've got a CodeIgniter based application with multiple modules and with a custom autoloader (and loader) to load namespaced models, libraries, interfaces, traits. Now I have to write unit tests for this application and most of my models and libraries are testable by injecting their dependencies individually, but the controller classes are not injectable. What I want to do is mocking the functionalities of the Controllers by override-ing the core parts with mocks. I want to be able to inject the depended classes within the CI_Controller and in my Controller.

There are a few possible solution to the problem:

narfbg commented 7 years ago

And a controller __construct( $dependency ) is calling parent::__custruct( $dependency ) or it's calling the parent::__construct(); ?

It calls whatever you call.

If nothing affect the the CI_Controller::__construct() then why we discussing about that?

Because extending base class functionality is not the same as changing how the class is instantiated in the first place!

I don't recommend that you do this, but if you already can and it does the job - how is it unnecessary, and why are you here at all?

If you have a spoon and a bucket to deliver water from one palce to an other you will use the spoon because eventually it's doing the same?

This is the most absurd analogy I've ever read, and if you're talking about performance - it's not even true; adding code for hooks adds more overhead, not reduce it.

I could hide the new $class(); and add my one with the current hooks but that would be a bad solution, because it would still generate the error message about mismatching paramethers on the class but after that I could at least do my custom one. It would be way easier if I could override it for my own needs.

What error message?! How's it even possible that you write that much text and still not explain what you're doing. I still have NO idea what you're trying to say.

That's our problem to solve in the future, not one for your custom hooks.

When? I waiting for the CI4 at least a year now and it's nowhere near to a preview version. Until I cannot use a fresh version I have to stick with the stable one but I need the new functionalites for creating better applications. I could use an other framework as well, but that's not helping at all.

Whenever it happens. Nobody is paid to do this. Use another framework.

You can easily add an autoloader, or pre-load traits and interfaces from inside a hook.

Yeah, so that's a job of my custom hooks but adding custom parts to the $CI = new $class(); is not fullify the "modify the inner workings of the framework without hacking the core files" condition so I shouldn't want that.

Correct. Some things should be done and some shouldn't. But even more importantly: some things are obvious and your use case is unknown.

There's literally nothing stopping you from using abstract classes.

Try load an abstract class in the autoloader then, don't tell me that only way to use abstract class is to require them or load them by autoloader.

That is the only way AND THERE'S A FUCKING REASON FOR IT: the loader does file inclusion and class instantiation to spare you an extra step. Abstract classes are not instantiable.

This way I cannot load my app dependencies before I want them use it with the core system. (but I did solved the problem with an Autoloader)

So there's a way and you already do it, but also somehow there's no way?! Are you intentionally trolling me or something?

Formerly I did maked a few pull requests to adding namespace and abstract class handling but I was strictly rejeted because that functionality is not part of the current version and I have to wait for the next version.

Namespaces would require a major overhaul and are certain to introduce BC breaks. Read above about abstract classes.

So because I need that functionality I moved the code to the core classes and I override the system by that, that's what I accepted, that no new features will allow into the 3.x version.

We do add new features, we don't add backwards-compatiblity breaking features.

Yet, somehow you've accepted "no new features", but here you want a new feature ... because "no new features"?!


Okay so I've got a CodeIgniter based application with multiple modules and with a custom autoloader (and loader) to load namespaced models, libraries, interfaces, traits.

So ... you've added modules, namespaces and something to do with traits - all things that CI isn't built to handle - and now you want to replace the class instantiation itself.

You're literally replacing the entire framework.

Now I have to write unit tests for this application and most of my models and libraries are testable by injecting their dependencies individually, but the controller classes are not injectable. What I want to do is mocking the functionalities of the Controllers by override-ing the core parts with mocks. I want to be able to inject the depended classes within the CI_Controller and in my Controller.

Finally! A use case! Why did I have to shout at you instead of starting with this in the first place?

There are a few possible solution to the problem:

  • I could accept the dependencies in constructor but with default NULL so the class could be created without any dependencies. This could make the DI work but it's I couldn't use Interface as an input there without adding new features to my autoloader.

Again, I don't get what you mean here at all.
I don't blame you for the bad English, but at least keep in mind that I don't know your code.

  • I could move the dependency requests to a initialize() method where I would request the dependencies and I could call it on my base controller, I could add it's functionalities as it was a contructror, but it's a seperate method whichs should be public but not callable by URL or CLI, that would need some magick method to hide it.

This already exists - prefix the method with an underscore. Or make it a non-public method and use Reflection in your tests.

Seriously, not only are we having this absurd conversation, but now it turns out we're having it because you haven't read the manual.

  • I could use an exists DI library, but for that I need to learn a complete new system and adapt it and it maybe solving my problem but it's time comsuming to at least try them because CI is not perapared for this kind of job.
  • I could add a hook to the CodeIgniter bootstrap where I could override the declaration of the controller class so I could add the dependencies automaticley everywhere.
  • I could add DI functionality to the CodeIgniter bootstrap itself, which could be a new feature which wont be accepted like my previnous pull requests.

Refer back to "I want a new feature because you don't accept new features" ...

gxgpet commented 7 years ago

@AdaLiszk Let's settle down a bit and see what you already have:

Any structures/architectures beside these will most probably change framework's codebase and that means you are already developing a custom application, rather than a CodeIgniter application.

adaliszk commented 7 years ago

@gxgpet Finally a meaningful Answer.

All I wanted to do is add Dependency Injection on Controller's constructor like this:

class Articles extends CI_Controller
{
    private $articles, $comments;
    public function __construct( ArticlesInterface $articles, CommentsInterface $comments )
    {
        parent::__construct();
        $this->articles =& $articles;
        $this->comments =& $comments;
    }

For achieve that I need something like this inside the CodeIgniter.php:

$CI_Contsruct = new ReflectionMethod( $class, '__construct' );
$Dependencies = $CI_Contsruct->getParameters();
$Parameters = [];

foreach($Dependencies as $dependency)
{
    $dependencyName = $dependency->getClass()->name;
    $dependencyName = str_replace('CI_', '', $dependencyName);

    $Parameters[] = load_class($dependencyName, 'core');
}

$CI = new $class( ...$Parameters );

I do NOT want to use the CI_Loader class because that's not handling for me the Interfaces if I'am not override it's functionality also It's allowed to use everywhere, not only in a fixed method.


If you need to have a "pre-call" method, then _remap is for you;

Is that called before __construct() ? if it's not then that's not what I want (but it's nice to have).

If you want to add an extra layer between CI_Controller and your controllers, then MY_Controller (or whatever) is for you;

I'am already using them, it's a great way to override core functionality when it's needed and add applicaton related functions.

If you want to use any namespaced/traited/abstracted/whatever external libraries, then CI has Composer support per application;

Yep, Im definetley using that, basicley I don't even need "library", I'am only using that to store non-data related classes which are specific to the project like a JSON_RPC client/server class.

If you want to create components/modules inside your CI application, then you should use libraries or helpers, both being CodeIgniter's features.

Nope, with this request I don't want that. ... but CI is almost capable handling HMVC modules, easy to add prefix directories to the CI_Router and to the CI_Loader, it's almost done and working properly in the system and I dont know why isin't part of the system yet.

Any structures/architectures beside these will most probably change framework's codebase and that means you are already developing a custom application, rather than a CodeIgniter application.

And I dont understand why is that a problem? I want to customize, add features to the CodeIgniter trough it's own interfaces like the core classes and hooks, from my perspective CI is a highly configrable framework where I could extend the basic functionality based on the project and I dont have to change the project workflow to fit to a framework, that's what I like in CI.

I see that there is a interface to do my DI manually but it's not trough a _getDependencies() method but that will only work until somebody does not replace MY_Controller and forgott to call it in a constructor. It would be way nicer to have a hook to implement it then in that way I could make or use an already complete dependency handler in a seperate compozer library.


Anyway it would be nice to have an override like with the Output, but if it's okay to override the complete output but not the router determinated class declaration then let it be. It's a shame that the system has great hooks and override abblitites by core classes but adding 3 line of code because it's has a potential to replace the core controller is too much.

gxgpet commented 7 years ago

@AdaLiszk It's not really a meaningful answer, but rather a summary of CI's user guide. :)

HMVC and any other stuff related to this is out of CodeIgniter's scope.

And I dont understand why is that a problem? I want to customize, add features to the CodeIgniter trough it's own interfaces like the core classes and hooks, from my perspective CI is a highly configrable framework where I could extend the basic functionality based on the project and I dont have to change the project workflow to fit to a framework, that's what I like in CI.

In this case, you are building a custom application and the changes shouldn't be regarded as being necessary to be added to CI's codebase.

If you are building a custom application, then you should seek help on forums, there should be a lot of people ready to help you, probably having your problems as well in the past.

adaliszk commented 7 years ago

@gxgpet Does not matter anymore, I decided to leave CodeIgniter, because I need to catch up to the cutting edge technologies and if a 3 line modification is that hard (without actually having an opinion) then it's not that framework what I want to use.

gxgpet commented 7 years ago

@AdaLiszk Personally, I'm sorry to see you going, but take the best decision for you.

The only point here is that if you can make a "3 lines change" and get your desired results, then why not doing it directly on your project code? :)

And as I said, if you want to share your findings of how could we use CI differently that it is intended, you are free to do it and also ask help (if you need) on forums.

adaliszk commented 7 years ago

@gxgpet: The only point here is that if you can make a "3 lines change" and get your desired results, then why not doing it directly on your project code? :)

Giving layers to layers to layers just to not touching the first one is showing in memory usage and runtime. It's a really big diference in performance. With the changes the code performance still do ~1k request per second but with a wrapper it's going down to 800-600, If I have a big API service it's does matter how fast the engine itself.