Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed issue #1076 #1081

Closed
wants to merge 5 commits into from
Closed

Fixed issue #1076 #1081

wants to merge 5 commits into from

Conversation

marcosgdf
Copy link
Contributor

Fixed issue #1076

@marcosgdf
Copy link
Contributor Author

So...? The issue has already been closed... This should be approved...

@narfbg
Copy link
Contributor

narfbg commented Mar 19, 2012

There's a conflict between your version and the one in EllisLab's repository. You'll have to sync any changes from it first.

@marcosgdf
Copy link
Contributor Author

What about now?

@narfbg
Copy link
Contributor

narfbg commented Mar 19, 2012

Yes, it can be merged now.
But why is _set_routing() execution moved after _set_overrides()?

@marcosgdf
Copy link
Contributor Author

Because _set_overrides() sets the params and should be set in order to _set_routing() detect them

CodeIgniter/system/core/Router.php - line: 510

if (isset($routing['function']))
        {
            $routing['function'] = ($routing['function'] == '') ? 'index' : $routing['function'];
            $this->set_method($routing['function']);
        }

@narfbg
Copy link
Contributor

narfbg commented Mar 19, 2012

_set_routing() doesn't seem to detect anything but settings from application/config/routes.php. I haven't tested it, but to me it looks like this would effectively eliminate any overrides.

@marcosgdf
Copy link
Contributor Author

Routing is set in index.php, line 99-105. The routing array is passed to set_overrides() and then is used in set_routing()

If you want to reproduce it, check issue #1076 and you'll see the problem...

@narfbg
Copy link
Contributor

narfbg commented Mar 19, 2012

OK, so I set $routing['directory'] to 'test', then I enable query strings and go to http://url/?d=blah. Line 112 in system/core/Router.php will set the routing directory to 'blah', no matter that I have set $routing['directory']. Same goes for classes and methods as well.
I don't see any checks in _set_routing() for if the directory, class or method are already set.

@marcosgdf
Copy link
Contributor Author

But that's only if you enable query strings... If you don't enable them, then you'll face the problem.

You set $routing['directory'] in the index.php file, then /system/core/CodeIgniter.php line 177 passes the array to _set_overrides, which does $this->set_directory($routing['directory']) /system/core/Router.php line 502.

Then it goes to _validate_request() (system/core/Router.php line 272) which is supposed to check if the controller exists.

@narfbg
Copy link
Contributor

narfbg commented Mar 19, 2012

... and it's exactly _validate_request() that will overwrite anything that was set using the $routes array.

@marcosgdf
Copy link
Contributor Author

So...?

I'm Spanish and my English is not that good.. so I'd appreciate if you can clarify what do you try to tell me...

@narfbg
Copy link
Contributor

narfbg commented Mar 20, 2012

I'm trying to tell you that _set_overrides() is supposed to be run after _set_routing().

What _set_routing() does is to trigger the logic that decides which directory/class/method should be run. It works on its own and doesn't take any overrides in mind.
_set_overrides() on the other hand is designed for one simple purpose in mind - to set hard-coded values, no matter what the router has decided to do before that.

@marcosgdf
Copy link
Contributor Author

OK. But if _set_overrides() is called AFTER _set_routing(), _validate_request() won't know the directory to search for the controller, and that's what is causing the problem.

@narfbg
Copy link
Contributor

narfbg commented Mar 20, 2012

If you've set $routing['directory'] - it will be set as the directory to include from, no matter what. It doesn't need validation.

@marcosgdf
Copy link
Contributor Author

Yes it does, in Router line 272, 278, 287.... That's the problem that I specified in bug #1076...

@narfbg
Copy link
Contributor

narfbg commented Mar 20, 2012

Now we're just repeating the same thing over and over again here.
I didn't write that and I don't want to argue, but I'm pretty sure that it is supposed to be this way ...

@philsturgeon @ericbarnes ?

@toopay
Copy link
Contributor

toopay commented Mar 21, 2012

Agree with narf. @marcosgdf i think issue #1076 could be resolved by some mod_rewrite / htaccess tweak rather than routes, since subdomain however, was considered as different domain and ideally should be treated as two different application which shared Codeigniter system. But despite i encourage the way someone maintain their application across different domain like that, you could still use your own approach on issue #1076, with some rewrite rule like :

RewriteEngine On 
RewriteBase / 

RewriteCond %{HTTP_HOST} ^foo.domain.com [NC] 
RewriteCond %{REQUEST_FILENAME} !(foo)/ 
RewriteRule ^(.*)$ index.php/foo/$1 [L]

RewriteCond %{REQUEST_FILENAME} !-f
RewriteCond %{REQUEST_FILENAME} !-d
RewriteRule ^(.*)$ index.php/$1 [L] 

This way, some request to http://foo.domain.com/something/weird would be get response from method weird at controllers/foo/something.

@marcosgdf
Copy link
Contributor Author

That's a posibility, but I'm not creating any feature, I mean, I'm just fixing CodeIgniter's implementation of that feature, which doesn't work well.

@narfbg narfbg closed this Oct 5, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants