Possible CI query string bug
Development and Programming
(manual repost from EE2 tech support - I’ve nuked the other thread)
I decided I should probably get other people’s opinion before filing an official bug report, so here I am.
I have noticed that putting a single query string element on the end of a URL causes a 404, but adding multiple elements (joined with an &) works correctly.
After doing some hunting around I’ve discovered that in the case of it failing, the single query string element is set as the URI string. If that element matches a template group/page then it will load that page, otherwise it will 404.
This led me to take a look at where and how the URI string var is set (in _fetch_uri_string() in CI’s URI.php, ~ line 69). I discovered this line:
if (is_array($_GET) && count($_GET) == 1 && trim(key($_GET), '/') != '')The count($_GET) of course being the culprit in this case. If there is only 1 element then it does something different and “weird”. The intention here is “it is simplest just to build the URI from the zero index of the $_GET array”, so if we have a single value, then use it as the page.
This is all well and good, especially if you are wanting to avoid the query string which is the CI/EE thing, but for there to be a difference between ?var1=foo and ?var1=foo&var2=bar is not helpful.
My suggestion therefore is to check for a value to the element to see whether this is a page or a variable, rather than simply grabbing the key. Something like:
if (is_array($_GET) && count($_GET) == 1 && trim($_GET[key($_GET)]) == '' && trim(key($_GET), '/') != '')Interestingly, this would also fix the need for EE to subclass _fetch_uri_string() to obtain the session variable so CI doesn’t use it as the page “/S/”
Thoughts? Is this is a valid bug (and fix) worth reporting?
I was super confused when I first ran into that too. It’s expected behaviour (and from reading the EE release notes, the $_GET variable is not accessible in version 2.3 anyway).
The thing is, some servers don’t support PATHINFO (e.g. “index.php/group/template”), so people rewrite their index.php like this
index.php?group/template
Then if there is only one GET variable, EE treats it as a path, which is why you see the 404 error. Also be aware that this behaviour changes depending on the $config[‘uri_protocol’] variable - sometimes EE is configured to always use this query string behaviour, even if you have more than one GET variable.
Anyway, as long as you are aware of this “bug”, you can always work around it by adding meaningless variables to your query string. Just be aware that you should probably avoid using query strings on the EE front end if at all possible.
(sorry for the late reply… been very busy!)
I was super confused when I first ran into that too. It’s expected behaviour (and from reading the EE release notes, the $_GET variable is not accessible in version 2.3 anyway).
Yes, but expected doesn’t make it “right” if there is an alternative. (Which is the reason I’m asking… is this alternative valid?)
However, the $_GET variable not even being available under 2.3 would make this discussion mute of course. I’ll have to take a look at the release notes.
The thing is, some servers don’t support PATHINFO (e.g. “index.php/group/template”), so people rewrite their index.php like this index.php?group/template
Yes, but that would still work as normal with my “fix” as there is no value to the $_GET variable (‘group/template’ is the key in that case).
Anyway, as long as you are aware of this “bug”, you can always work around it by adding meaningless variables to your query string. Just be aware that you should probably avoid using query strings on the EE front end if at all possible.
Well yes, it is possible to work around, but messy.
The way that EE and CI handles query strings is wrong, but probably will not change. To use query strings just adjust your $config[‘uri_protocol’].
The way that EE and CI handles query strings is wrong, but probably will not change.
Indeed… but I think it is still worth pointing out ways it can be improved.
To use query strings just adjust your $config[‘uri_protocol’].
Unfortunately, this can’t be relied on as this particular development is for a module with the intention of distribution, and I can’t ask people to enable that var in their config purely to use my module.
I can of course get around it by having a (virtually) endless list of properties for certain tags which then build and return an EE/CI safe URI string, but this is unnecessary obfuscation and isn’t brilliant for SEO. I am most probably going to go this way anyway for compatibility, but I figure it is still worth reporting.
Indeed… but I think it is still worth pointing out ways it can be improved.
Ahhh, I remember having that level of optimism 😉 Seriously though, if you want something to change in EE, it takes serious campaigning. i could write a book on things I feel could be improved, but the pace of EE development is intentionally restricted, because most people would rather have a stable product than bleeding edge features and changes.
Have you tested your module in 2.3? From what I understand, the $_GET variable is completely inaccessible (though haven’t tested this yet). We use it currently, and I do need to get some users to adjust their $config[‘uri_protocol’], although generally AUTO is fine, and I always make sure there are at least two GET variables in any query string (even if I just need to append &return=1)
Having said that though, you should not be using GET variables in your EE plugin. Find another way. Either use an action URL, or encode the parameters as segments ($this->EE->uri->uri_to_assoc() is awesome - http://ellislab.com/codeigniter/user-guide/libraries/uri.html)
Have you tested your module in 2.3? From what I understand, the $_GET variable is completely inaccessible (though haven’t tested this yet).
No, I haven’t obtained a copy of 2.3 yet. I’m actually developing this module on behalf of a client and they haven’t provided me with the update as of yet. Hopefully in the next couple of days…
We use it currently, and I do need to get some users to adjust their $config[‘uri_protocol’], although generally AUTO is fine, and I always make sure there are at least two GET variables in any query string (even if I just need to append &return=1)
Yeah, that is a good workaround (and is exactly what my app is doing - although that’s likely to change with 2.3), I just consider it messy that’s all.
Having said that though, you should not be using GET variables in your EE plugin. Find another way. Either use an action URL, or encode the parameters as segments ($this->EE->uri->uri_to_assoc() is awesome - http://ellislab.com/codeigniter/user-guide/libraries/uri.html)
<rant> I do understand that very few people (if anyone) can answer this (and for that reason it is mostly rhetorical) but the question is why should GET request variables be avoided. “Because it is the EE/CI way” is not a valid reason, we just learn to live with it.
Sure, for a dynamic system based around a single “page” (index.php) for SEO reasons and general nicety, page naming (and all the necessary parts to specify that specific page) should be part of the directory structure rather than the query string.
The flip side of that is, query parameters should be specified in the query string. Specifying them as part of the directory structure is wrong from the point of view of SEO. Parameters being passed to a page for processing (eg. shopping cart) do not define that page and should not cause that page to be indexed as such. Using an action URL is a messy alternative if you are not doing a redirect (which is unnecessary) immediately afterwards.
If nothing else, it looks far nicer for links to have a descriptive page rather than ACT=xx with a list of parameters.
Anyway, I should probably not continue along this line as this a rant, and one that appears to have been had on this board many, many times before. </rant>
If they are disallowing the usage of query strings in front end templates then that is totally unacceptable and it demonstrates a flawed understanding of HTTP and web development.
Actually I had a chat to the team at EECI and it sounds like they have fixed this whole thing up in 2.3.1. I haven’t done any testing yet so will see what we uncover.