Skip to content
This repository

Bug with 404_override #1062

Closed
Razican opened this Issue February 19, 2012 · 8 comments

5 participants

Iban Eguia Timothy Warren Shane CroNiX Andrey Andreev
Iban Eguia

I have configured 404 override to work with my controller Error. That controller has a _remap method. This is the controller:

<?php defined('BASEPATH') OR exit('No direct script access allowed');

class Error extends SPS_Controller {

    public function _remap($error)
    {
        if($this->uri->segment(1) != 'error') redirect('error/'.$error);

        switch($error)
        {
            case '404':
                header('HTTP/1.1 404 Not Found');
                $this->load->view('404');
            break;
            default:
                redirect('error/404');
        }
    }
}


/* End of file error.php */
/* Location: ./space_settler/controllers/error.php */

When I access a class which does not exist, it works perfectly. The problem comes when the class exists but the method does not. In that case it throws this exception:

Severity: Warning
Message: call_user_func_array() expects parameter 1 to be a valid callback, class 'Error' does not have a method '404'
File: core/CodeIgniter.php
Line: 359

I understand it has some kind of problem using _remap method.

Timothy Warren

I think this is normal behavior, perhaps unexpected. A workaround would be to create a __call() method that does the 404 page. That way you get around the missing method.

Iban Eguia

But why is it normal to throw an exception? it should show the same 404 page if /class/method does not exist and if /class does not exist

Timothy Warren

@Razican It's because you are using the _remap. I could be wrong, but I believe that completely overrides default routing behavior for that controller. If you can fix the throwing of the error in the system classes, feel free to submit a pull request. I've personally never used _remap, so I'm a little fuzzy on it's behavior, but this is from my (probably limited) understanding.

Shane

the issue is in this section of CodeIgniter.php - https://github.com/EllisLab/CodeIgniter/blob/develop/system/core/CodeIgniter.php#L321

If the controller exists but the method does not, _remap() gets called before the check for a valid method in the controller. Then when the method check fails, the 404 override is loaded but _remap() for the 404 override is ignored.

It's not documented but judging from the code I'm pretty sure the 404_override route expects it to be of the form "controller/method" https://github.com/EllisLab/CodeIgniter/blob/develop/system/core/CodeIgniter.php#L336 :

<?php
$x = explode('/', $RTR->routes['404_override'], 2);
$class = $x[0];
$method = (isset($x[1]) ? $x[1] : 'index');

This is also how the router sets it https://github.com/EllisLab/CodeIgniter/blob/develop/system/core/Router.php#L327.

Since your final destination always ends up being 'error/404' does it work if you do it like this?

<?php
$route['404_override'] = 'error/index';
<?php defined('BASEPATH') OR exit('No direct script access allowed');

class Error extends SPS_Controller {

    public function index()
    {
            header('HTTP/1.1 404 Not Found');
            $this->load->view('404');
    }
}

/* End of file error.php */
/* Location: ./space_settler/controllers/error.php */
CroNiX

I kind of think this is a trivial issue that will only affect this particular case of using _remap() within the 404_override controller.
It would be best to use an actual method like @bubbafoley suggests and then those workarounds aren't needed.

I also think it wouldn't be very good (for SEO) to have a redirect to a controller on a 404 because then the url will be that of the 404_override controller instead of the url that caused the actual 404.

Perhaps it could/should be mentioned in the user_guide (in controllers/remapping) that _remap() should not be used in the 404_override controller. The default example in routes.php for 404_override shows the route using a controller/method, so that part should be ok.

Alan Jenkins sourcejedi referenced this issue from a commit in sourcejedi/CodeIgniter May 11, 2012
Alan Jenkins Don't create the controller twice when using 404_override
2.1.0 tried to make it safe to create the controller twice.
However I still noticed a problem when loading models
in the constructor of MY_Controller (using db auto-connect).
This caused an error "Undefined property: Page_Not_Found::$db"
in system/core/Model.php.

I wouldn't be surprised if there were other lurking issues.
(E.g. I notice the hooks called "pre_controller" and "post_controller"
... those names could be misleading in the case where the controller
was created twice).

This also happens to affect #1062
<EllisLab#1062>
(404_override with _remap() method doesn't work).
_remap() should now get invoked whenever the 404_override
is used, instead of only in some cases.
e7d8435
Iban Eguia

Could that be merged?

Andrey Andreev
Collaborator
narfbg commented June 25, 2012

@Razican I'm not much into the routing stuff, so I'd rather leave it to someone else to review and decide, but either way - this is not a pull request, there's nothing to merge.

Iban Eguia

Yes, I know, I meant if it would be added to CodeIgniter. It's OK if someone else reviews it.

Iban Eguia Razican closed this October 30, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.