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

Already on GitHub? Sign in to your account

404 and router fixes #1352

Closed
wants to merge 13 commits into
from

Conversation

Projects
None yet
3 participants
Contributor

sourcejedi commented May 11, 2012

Don't create the controller twice when using 404_override

That's the main commit anyway. While I was rearranging this, I tried to avoid breaking anything. Since the boundary of what would work as expected was a little ragged, I had to shore it up a bit first :).

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 with the same root cause. (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).

sourcejedi added some commits May 8, 2012

@sourcejedi sourcejedi Comment fix for the $rsegments field 05acc38
@sourcejedi sourcejedi Remove unused field $error_routes 8f7d2df
@sourcejedi sourcejedi Fix _set_request() to call uri->_reindex_segments()
Each call to _set_request() requires a follow-up call
to uri->_reindex_segments().  It is not optional.
So _set_request() should be calling it itself.

This fixes the following buggy call sequence:

 _set_routing()
   _parse_routes()
     _set_request()
       _set_default_controller()
         _set_request()
         _uri->_reindex_segments()
  _uri->_reindex_segments()

where we end up calling _uri->reindex_segments()
more than once.
922ec05
@sourcejedi sourcejedi Remove _reindex_segments()
_reindex_segments() makes it hard to re-organize the Router code;
we have to make sure to call it only once.  Replace it with a
more sensible _set_rsegments() method.
425fd9f
@sourcejedi sourcejedi Make sure _set_request_novalidate() always sets the method
I.e. it should replace any old value.

(See next previous commit message for an example where this caused
a problem.  In future, this will be useful when a request has to
fall back to the 404_override controller).
3b7d997
@sourcejedi sourcejedi Don't strlower() the 'default_controller' route
strlower() isn't used anywhere else in the router,
so there's no reason it should be needed for
the default route.
708cfe3
@sourcejedi sourcejedi Add _set_404() method to the Router
Much the same as _set_default_controller().
This will be used to unify 404 handling.
045236b
@sourcejedi sourcejedi Fix request validation for the 'enable_query_strings' case
Under 'enable_query_string' we set the directory, class
and method directly.  Don't smush them into an array
and pass them to _validate_request(), because then
it has to guess whether a directory was used or not.

This commit will also preserve method arguments set in
the 404_override route, if we end up using that.
(Instead of silently ignoring them).
ff61081
@sourcejedi sourcejedi Fix _set_default_controller() not to look in sub-folders
_set_default_controller() sets the class and method directly.
That's ok, because we haven't actually defined what it
would mean to have a default route pointing to a sub-folder.
(Does the URL /directory/ cause us to look in a sub-sub-folder?
Could we get into an infinite loop?)

But then it goes ahead and call _set_request(), which may
try to look in sub-folders.  It would then re-set the class
 - and the method.  Unless the default route doesn't specify
a method, in which case it would set a new class but preserve
the old method, producing an inconsistent request.

Split the part of _set_request() we _do_ want into
_set_request_novalidate(), and call that instead.
5a63c68
@sourcejedi sourcejedi Check for missing 404_override controller file in _set_404()
This matches the behaviour of 404_override
in the CodeIgniter bootstrap file
(which triggers e.g. when the _method_ is missing).
3320b66
@sourcejedi sourcejedi is_callable() was fixed in PHP 5_0; remove workaround
The bug with is_callable() and private methods was fixed
some time in PHP 5.0.  We require 5.2.4, so we can switch
back to is_callable().

<http://uk.php.net/manual/en/function.is-callable.php>
<https://bugs.php.net/bug.php?id=29210>
d2de251
@sourcejedi sourcejedi Use the router's _set_404() in CodeIgniter bootstrap
I.e. if the method doesn't exist.  The router already
needs this code, so we don't need to duplicate it here.

This also improves consistency in the segments passed to
the 404_override controller.  (Both $this->uri->rsegments
and any extra function parameters should reflect the
404_override route.  The failed uri is available in e.g.
$this->uri->segments).
ffead8e
@sourcejedi sourcejedi 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
<#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

@philsturgeon philsturgeon commented on the diff May 23, 2012

system/core/Loader.php
*/
public function initialize()
{
- $this->_ci_classes = array();
@philsturgeon

philsturgeon May 23, 2012

Contributor

Whats happening here? Accident?

@sourcejedi

sourcejedi May 23, 2012

Contributor

On 23/05/12 18:20, Phil Sturgeon wrote:

 */
 public function initialize()
 {
  •    $this->_ci_classes = array();
    
    Whats happening here? Accident?

Intentional. The field does have an initializer, so the question is why
does initialize() set it again?

Answer: despite the function name and the comment, initialize() was
intended to be called more than once. When CodeIgniter.php used the
404_override route, CI_Controller was created twice, and initialize()
was called each time.

The second time round, initialize() had to reset the loader's idea of
which optional classes were loaded. Because those classes would have
been assigned to the first controller, and if the second controller
requests those classes, a new instance needed to be assigned to the
second controller.

With this commit, CI_Controller is only ever created once.

Alan

@narfbg narfbg added a commit that referenced this pull request Nov 2, 2012

@narfbg narfbg Router-related optimizations
An improved version of changes suggesed in PR #1352, and more specifically:

sourcejedi@8f7d2df
sourcejedi@d2de251

(thanks again @sourcejedi)
7d394f8
Contributor

narfbg commented Nov 3, 2012

Implemented (alternatively) most of the proposed changes.

@narfbg narfbg closed this Nov 3, 2012

Contributor

sourcejedi commented Nov 3, 2012

Great! I'm not deeply attached to the exact cleanups I put in. I was just trying to make sure the singleton fix didn't end up breaking anyone else's site.

Thanks for looking at my pull requests.

@nonchip nonchip pushed a commit to nonchip/CodeIgniter that referenced this pull request Jun 29, 2013

@narfbg narfbg Router-related optimizations
An improved version of changes suggesed in PR #1352, and more specifically:

sourcejedi@8f7d2df
sourcejedi@d2de251

(thanks again @sourcejedi)
ac7ee8f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment