Welcome Guest, Not a member yet? Register   Sign In
CI3 core code bug
#1

(This post was last modified: 01-12-2017, 03:58 PM by Ryuu. Edit Reason: Add info )

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


Messages In This Thread
CI3 core code bug - by Ryuu - 01-12-2017, 03:59 AM
RE: CI3 core code bug - by ciadmin - 01-12-2017, 08:30 AM
RE: CI3 core code bug - by ivantcholakov - 01-12-2017, 12:24 PM
RE: CI3 core code bug - by Ryuu - 01-12-2017, 03:26 PM
RE: CI3 core code bug - by ciadmin - 01-12-2017, 08:50 PM
RE: CI3 core code bug - by Ryuu - 01-12-2017, 09:41 PM
RE: CI3 core code bug - by ivantcholakov - 01-12-2017, 11:57 PM
RE: CI3 core code bug - by Ryuu - 01-15-2017, 08:35 PM
RE: CI3 core code bug - by ciadmin - 01-15-2017, 10:03 PM



Theme © iAndrew 2016 - Forum software by © MyBB