+- CodeIgniter Forums (https://forum.codeigniter.com)
+-- Forum: Development (https://forum.codeigniter.com/forumdisplay.php?fid=6)
+--- Forum: Issues (https://forum.codeigniter.com/forumdisplay.php?fid=19)
+--- Thread: CI3 core code bug (/showthread.php?tid=67076)
CI3 core code bug - Ryuu - 01-12-2017
CI3 main code seem to miss about procedural concept (not sure about CI2, I think it may be more but I didnt check it)
I found them when I migrate from CI2 to CI3 so that I have to adapt the code (which is not the problem).
I think you should revise the core code to make sure it is consistently fine, not just ok in the happy path.
For example, the code here:
...run hook...
require_once BASEPATH.'core/Controller.php';
/**
* Reference to the CI_Controller method.
*
* Returns current CI instance object
*
* @return CI_Controller
*/
function &get_instance()
{
return CI_Controller::get_instance();
}
CI3 introduce the 'hook' which -my guess- to solve many problem about CI3 autoloading is call by controller (which is not the case in CI2).
Now let say we use hook with before controller load. But we need some feature so we call 'get_instance'.
'get_instance' will be there but 'require_once BASEPATH.'core/Controller.php';' will not run yet so ..error 'CI_Controller' is not found.
*You can see many exact error in the same manner in google.
What's wrong here is that usual function will be loaded first when the file is readed but those execution code (aka. require_once) will not be run yet. It seem like some CI developer is not clear about this manner (in many language, not only PHP so it's not PHP false).
It will be the 'happy-path' if no-one call anything above CI (means CI not call any user code) but with some feature used, CI call them and it will have inconsistent result here. It is ok if this code is some user view code himself which he should just break elegantly. But this is the framework/core so it should not break if it can be avoid. ..maybe something like this (you may found the better solution)..
require_once BASEPATH.'core/Controller.php';
function &get_instance()
{
require_once BASEPATH.'core/Controller.php';
return CI_Controller::get_instance();
}
The other main point is about changing. I think it is too much. For the name sake it is understandable and easy to do for me I just write some line of code to convert my old code to the new one -I must do code for both CI2&CI3 at the same time with the same code so I prefer to code one place and just convert it automatically by my small app.
But some changes is hard to change such as those input/get result which change from false to null. It is ok for my part because I call them by my middle function (I can change it there at one place) but many of my team call input->get, some call isset($_GET..) is more happy. As a result, the one who uses framework have a hard time -many thousands of place- instead the one who not. This will encourage the people to go his own way instead to use the framework properly. Do you think so? I think you should have a config to be choice (constant or something) will be better than force all to change immediately with huge impact.
Sorry that I can report only few place, there's no time to list it all.
I can do something as my workaround but I think I should inform CI team too for better CI so no need to be rush, just FYI.
-----Cant reply so I will reply here-----
@ciadmin
You misunderstand the point here.
The intended behavior is as is I describe so that I mean I understand the change (and have no problem with it)
But it do the job incorrectly in some case as in example.
Let me make another example (sorry if you feel it is too long :P)
If you do the cart system/bank system there's many way to add subtract calculation.
First, normal sum all each time and save as is -this is accepted best practice-
Second, save the change such as begin with 10, -5, +3,... (First method will keep 10, 5, 8,...)
The second method will be wrong if some record is missing so it is known to be not recommend
*I'm not sure if every one read here will have code design or math background so I may describe it too much.
You see? same 'intended behavior' but the second is not well thought, result to error there.
*In fact, I myself know the system use logic as this, it uses addition/subtract way and save to cache, and it often incorrect when user do something simultaneously e.g. 10-5 and when user do +3 that -5 still not done, result to 10+3=13 instead of 10-5+3=8
***Above CI example problem is CI is called 'get_instance' and CI call CI_Controller 'by itself' when it is not loaded yet=error
@ivantcholakov
You understand my point well, and I'm very agree with your opinion (yes just mine too :P) but one point
I think it is very reasonable for someone to get_instance when he/she hook
For example, simple input query (get/post). They see it more proper to call via CI instead of $_GET/$_POST which is not XSS filtered and there are more plenty others. And it is not hard to fix, as in my example solution: add require_once BASEPATH.'core/Controller.php'; there should not break anything, spl_autoload_register is interesting another solution but not CI current style.
I just use what CI provide, simple hook/get_instance (which function exists there) so it shouldn't error for its code IMHO.
For my detailed solution: I just add require_once BASEPATH.'core/CodeIgniter.php'; in the hook file
*I didn't use any sophisticated thing and core is in other path such as $bonfire_path by that link
It is there at 'BASEPATH.'core/CodeIgniter.php'' but it is still not executed (as below, more description)
----------
@ciadmin sorry for again TL but please read this is CI code
1...ci run hook...
2require_once BASEPATH.'core/Controller.php';
3 function &get_instance()
{
4 return CI_Controller::get_instance();
some dev. may think it will always run 2>3>4 but that is not procedural concept (of many language and importantly- PHP)
it will read 3 (because it is function) > do 1 (and for example if 1 call get_instance which is very valid) > do 4
incorrect CI code there it require CI_Controller (controller.php) but it place at '2' which is not executed yet, see the point?
> and lastly it will do 2 (which is too late, error before)
-I load it above there so it MUST exist.. no that is not true > this is plain error point and error idea, not about how user do
As I say, I didn't expect anything to be fixed but it should be, really. IMHO
And I'm not complain for anything to be changed so we should see the fact without bias.
Just for better CI, cause I think proper feedback helps the code better.
----------
RE: CI3 core code bug - ciadmin - 01-12-2017
Hmmm. The behavior you describe is exactly the intended behavior for CodeIgniter 3.
The **get_instance()** function is provided so that the CI super-object can be accessed outside the main processing flow.
"Hooks" are an implementation of an "intercepting filter" design pattern, and a normal part of a framework.
RE: CI3 core code bug - ivantcholakov - 01-12-2017
@Ryuu
About get_instance(): CodeIgniter 2 & 3 uses the "God object" pattern, which at its early (no namespaced) times seems to be a natural choice. But IMO once a God object is used, it should be created first, and as the system bootstraps, the object's properties should have been added/modified gradually.
Currently, several classes are initialized first - Config, Loader, Router, etc., and after routing is resolved, the God object CI_Controller gets created with its full capabilities, however moment of creation is quite late. This is an architectural mistake (my opinion). Here you are additional thoughts: https://github.com/bcit-ci/CodeIgniter/issues/3191#issuecomment-54097544 Fixing this mistake would require lots of refactoring and testing, today this maybe is worthless.
If you badly need to do some hacking about system bootstrapping, see this comment and do something similar: https://github.com/bcit-ci/CodeIgniter/commit/ed86ee14f3a36de1034b8fa19ff6d41aeb428a93#commitcomment-6985684
RE: CI3 core code bug - Ryuu - 01-12-2017
@ciadmin
You misunderstand the point here.
The intended behavior is as is I describe so that I mean I understand the change (and have no problem with it)
But it do the job incorrectly in some case as in example.
Let me make another example (sorry if you feel it is too long
)If you do the cart system/bank system there's many way to add subtract calculation.
First, normal sum all each time and save as is -this is accepted best practice-
Second, save the change such as begin with 10, -5, +3,... (First method will keep 10, 5, 8,...)
The second method will be wrong if some record is missing so it is known to be not recommend
*I'm not sure if every one read here will have code design or math background so I may describe it too much.
You see? same 'intended behavior' but the second is not well thought result too error there.
*In fact, I myself know the system use logic as this, it uses addition/subtract way and save to cache, and it often incorrect when user do something simultaneously e.g. 10-5 and when user do +3 that -5 still not done, result to 10+3=13 instead of 10-5+3=8
***Above CI example problem is CI is called 'get_instance' and CI call CI_Controller 'by itself' when it is not loaded yet=error
@ivantcholakov
You understand my point well, and I'm very agree with your opinion (yes just mine too
) but one pointI think it is very reasonable for someone to get_instance when he/she hook
For example, simple input query (get/post). They see it more proper to call via CI instead of $_GET/$_POST which is not XSS filtered and there are more plenty others. And it is not hard to fix, as in my example solution: add require_once BASEPATH.'core/Controller.php'; there should not break anything, spl_autoload_register is interesting another solution but not CI current style.
I just use what CI provide, simple hook/get_instance (which function exists there) so it shouldn't error for its code IMHO.
For my detailed solution: I just add require_once BASEPATH.'core/CodeIgniter.php'; in the hook file
----------
@ciadmin sorry for again TL but please read this is CI code
1...ci run hook...
2require_once BASEPATH.'core/Controller.php';
3 function &get_instance()
{
4 return CI_Controller::get_instance();
some dev. may think it will always run 2>3>4 but that is not procedural concept (of many language and importantly- PHP)
it will read 3 (because it is function) > do 1 (and for example if 1 call get_instance which is very valid) > do 4
incorrect CI code there it require CI_Controller (controller.php) but it place at '2' which is not executed yet, see the point?
> and lastly it will do 2 (which is too late, error before)
----------
RE: CI3 core code bug - ciadmin - 01-12-2017
My background is math & programming, and your example doesn't help explain anything. Your original post is getting longer, as you add more & more to it, even though replied to subsequently. I am glad that ivantcholakov understands you, and will leave the help to him
RE: CI3 core code bug - Ryuu - 01-12-2017
I didn't mean to be offend really. Sorry if you feel that way it means for the other who interest too, not to say you've no knowledge or something like that.
In sum and short: in CI code, line 4 may run without line 2 executed first lead to the bug
2require_once BASEPATH.'core/Controller.php';
3 function &get_instance()
{
4 return CI_Controller::get_instance();
but this
2require_once BASEPATH.'core/Controller.php';
3 function &get_instance()
{
2.2require_once BASEPATH.'core/Controller.php';
4 return CI_Controller::get_instance();
will guarantee that 2.2 is called before 4 as example about uninterrupt/'atomic operation'
*For the case of edit my post, is to add my reply because I cant submit reply at first then I edit my post.
It's wait for mod to approve but those popup is very fast disappear and redirect back so that I dont know why I cant reply at all, pardon me for my duplication. You seem to be angry that why you see me as 'why you edit your post then reply it, for jokingly?' It's not that hard to understand if you open your mind. Please forgive me and read it without bias
RE: CI3 core code bug - ivantcholakov - 01-12-2017
@Ryuu
Make an alternative experiment. Within CodeIgniter.php cut this fragment of code:
Code:
// Load the base controller class
require_once BASEPATH.'core/Controller.php';and paste it after this piece of code:
Code:
/**
* CodeIgniter Version
*
* @var string
*
*/
const CI_VERSION = '3.1.3';Thus, you will be able to call CI_Controller::get_instance() earlier.
But this does not always mean that the returned value will be an object, because the controller instance gets created after routing has been resolved.
If you come with code that works for you, make the permanent hack for yourself as I described before and leave the original things as they are. More refactoring is needed, but nobody has the time for it.
Edit: For more playing where to move pieces of code see this heavily modified file: https://github.com/ivantcholakov/starter-public-edition-3/blob/v3.2.31/platform/create.php I hope, it would be helpful and I am out of here too.
RE: CI3 core code bug - Ryuu - 01-15-2017
Very thanks ivantcholakov,
I fix something nearly what you mention by insert require_once BASEPATH.'core/Controller.php'; in the hook file above the hook that use 'get_instance' which require CI_Controller as in my 2nd comment. The reason I put it in there is for easy to update CI to the new version -hence no need to edit it every update-, I usual do the neat code so I find the way to fix it without changing any CI code.
By the way, I report it so that it is fixed because usual who[1] call which[2] class should load it[2] itself[1].
So in this case, I expect CI team to fix it properly that get_instance proper load the CI controller file itself or atleast perceive it to prevent future same type of error.
But it seem like 'who cares' by admin so it's surely this bug is not reported to the team even though most of their team may willing to fix the bug (and may fix it fast because it is very easy). It's sadly to see this attitude from any kind of developer -programmer of not. Developer should be eager to tackle the problem even though it may feel bothersome sometimes, that's what called 'develop'.
RE: CI3 core code bug - ciadmin - 01-15-2017
CodeIgniter provides hooks, yes. *By design*, get_instance does *not* exist when "pre-system" hooks are called. That is the intent.
There is a "pre-controller" hook, which happens after all the core system classes have been instantiated, and at which point, *by specific design*, get_instance *does* exist.
If you want to write a hook that uses get_instance, shouldn't you be configuring it as a "pre-controller" hook.