EllisLab text mark
Advanced Search
2 of 10
2
   
Please consider querystring support in 2.x
Posted: 30 June 2010 08:00 AM   [ # 16 ]   [ Rating: 0 ]
Avatar
Joined: 2007-06-11
2987 posts
Crimp - 30 June 2010 11:57 AM

Why segments are better?

People can’t/don’t t read URLs and GET is largely to blame.

This “fact” was discovered when a popular blog post on the topic of Facebook login reached #1 on Google.

Angry comments soon filtered in about people not being able to log in and why did Facebook change the site and how the old one was much better.

Turned out many had no idea what the mumbo jumb in the location bar was and had developed a habit of totally ignoring it. To log in to Facebook, they went via “Facebook login” on Google and got accustomed to clicking the top result.

Segments open up URLs to become more readable, comprehensible and usable.

Let’s not confuse the conversation here by starting a “Query string v Segment URI” argument as that is irrelevant.

CI is designed to work with segments, great. We just need a way to easily throw in some ?foo=bar for Twitter, oAuth support, Facebook Connect, etc without recoding our apps or hacking it with hooks like mentioned above.

Let’s put our heads together and come up with a solution. “Go team!”

 Signature 

————————
Blog | Twitter | GitHub | BitBucket
————————-
PyroCMS - open source modular CMS built with CodeIgniter
PancakeApp - Simple, hosted invoicing/w project management

 
Posted: 30 June 2010 11:58 AM   [ # 17 ]   [ Rating: 0 ]
Avatar
Joined: 2008-11-04
4489 posts

There are several design decisions made that influence this (in 1.7.2):

The Input library assumes that ‘enable_query_strings’ == ‘allow_get_array’, which is not the case. The first one is your config ‘switch’ to decide if you want to use URI segments or the query string to define the controller/method load, the second one whether or not to erase the $_GET array. This two functions need to be split (and a config value needs to be defined for the latter).

The URI library (protocol == ‘AUTO’) assumes that if the $_GET array has values, they should be used for determining the URI string. Which is not the case, the ‘enable_query_string’ should be used to determine that. This has been correctly implemented in the Router class (_set_routing).

Structural solution for CI v1.7.2:
- add a new config variable ‘allow_get_array’ (boolean, default FALSE)
- in ./system/codeigniter/Codeigniter.php, move loading the Input class up, between the Config and the URI class
- in the Input library, create a new class variable ‘enable_query_strings’, and assign the config value to it
- in the Input library constructor, load and store the ‘allow_get_array’ value separate from the ‘enable_query_strings’ value
- in the URI library, only use $_GET if ‘enable_query_strings’ is TRUE

Diff for these changes in attachment.

I’ll have a peek at 2.0 as well…

 Signature 

Me: WanWizard.eu | My company: Exite | Datamapper: DataMapper ORM <= LOOKING FOR A NEW MAINTAINER!

 
Posted: 30 June 2010 12:39 PM   [ # 18 ]   [ Rating: 0 ]
Avatar
Joined: 2008-11-04
4489 posts

Checked the 2.0 code, looks like the same modifications will fix it there as well.

edit: Just did a test install of 2.0, and made the modifications. I can confirm the patch works for 2.0 as well.

I can confirm that a URL like http://test.mydomain.local/index.php/welcome/index?foo=bar adds ‘welcome’, ‘index’ to the URI segments array, and ‘foo’ => ‘bar’ to $_GET.

 Signature 

Me: WanWizard.eu | My company: Exite | Datamapper: DataMapper ORM <= LOOKING FOR A NEW MAINTAINER!

 
Posted: 30 June 2010 12:49 PM   [ # 19 ]   [ Rating: 0 ]
Avatar
Joined: 2008-11-04
4489 posts
Phil Sturgeon - 30 June 2010 10:36 AM

This is still in 2.0 along with the extra screwyness that is example.com/?/controller when you use anchor() or site_url().

Could you explain what this is about, so I can test if my patch doesn’t destroy current functionality?

 Signature 

Me: WanWizard.eu | My company: Exite | Datamapper: DataMapper ORM <= LOOKING FOR A NEW MAINTAINER!

 
Posted: 01 July 2010 06:32 AM   [ # 20 ]   [ Rating: 0 ]
Joined: 2006-03-21
220 posts

WanWizard, thanks, I will give that a try! If this works reliably and performance-wise, it would be a great enhancement for all CodeIgniter developers to be able to safely and easily use querystrings where it make sense.

 
Posted: 01 July 2010 02:50 PM   [ # 21 ]   [ Rating: 0 ]
Avatar
Joined: 2009-04-13
369 posts

A while back i submitted a pull request to EllisLab on BitBucket for v2.0.  Here is the commit: http://bitbucket.org/dhorrigan/codeigniter/changeset/8e4d17281982

Probably won’t get into the core, but hey…it was worth a try.

Dan

 Signature 

Blog | UhOh! | Formation | Query Strings in CI

 
Posted: 01 July 2010 03:11 PM   [ # 22 ]   [ Rating: 0 ]
Joined: 2006-03-21
220 posts

Does anyone here have good relations with EllisLabs to see if they would consider this?

 
Posted: 02 July 2010 12:14 AM   [ # 23 ]   [ Rating: 0 ]
Joined: 2006-03-21
220 posts

Does this look right for 2.0?

 
Posted: 02 July 2010 05:13 AM   [ # 24 ]   [ Rating: 0 ]
Avatar
Joined: 2008-11-04
4489 posts

Looks ok.

The changes for 2.0 are exactly the same as for 1.7.2.

 Signature 

Me: WanWizard.eu | My company: Exite | Datamapper: DataMapper ORM <= LOOKING FOR A NEW MAINTAINER!

 
Posted: 13 July 2010 06:49 AM   [ # 25 ]   [ Rating: 0 ]
Joined: 2010-03-30
23 posts
Phil Sturgeon - 30 June 2010 12:00 PM
Crimp - 30 June 2010 11:57 AM

Why segments are better?
CI is designed to work with segments, great. We just need a way to easily throw in some ?foo=bar for Twitter, oAuth support, Facebook Connect, etc without recoding our apps or hacking it with hooks like mentioned above.
Let’s put our heads together and come up with a solution. “Go team!”

I can’t agree more. 
If building your site from the ground up and the lack of query strings is rarely a problem (or hasn’t been for me ever).  Once you have to deal with other people’s code (especially my current nemesis Facebook) and you do have to hack about a bit.  Using Elliot Haughin’s new facebook library we’ve had a lot of success, but it’s still very much feels like hacking about, not clean coding.

My ideal solution would mean that segments worked via routes for pointing to controllers, methods and passing in data, but that tagging a query string on the end would then make these variables available to that controller/method and not end up in 404’s or routing problems.  We’ve nearly got this by using a combination of $config[‘uri_protocol’]  = “PATH_INFO”; and modifying $config[‘permitted_uri_chars’]

 Signature 


I run http://www.theapproachablegeek.co.uk and blog at http://www.coldclimate.co.uk

 
Posted: 13 July 2010 12:11 PM   [ # 26 ]   [ Rating: 0 ]
Avatar
Joined: 2008-11-04
4489 posts

That might work in your case, but I prefer not to fiddle with config values, that might have side-effects elsewhere.

On one of my servers p.e., PATH_INFO doesn’t work, no segments can be loaded and the application fails miserably…

 Signature 

Me: WanWizard.eu | My company: Exite | Datamapper: DataMapper ORM <= LOOKING FOR A NEW MAINTAINER!

 
Posted: 13 July 2010 01:28 PM   [ # 27 ]   [ Rating: 0 ]
Joined: 2006-03-21
220 posts

Wan, your solution works with no side effects?

 
Posted: 13 July 2010 02:29 PM   [ # 28 ]   [ Rating: 0 ]
Joined: 2010-07-13
1 posts
pbreit - 13 July 2010 05:28 PM

Wan, your solution works with no side effects?

Looks ok.

 
Posted: 13 July 2010 04:16 PM   [ # 29 ]   [ Rating: 0 ]
Avatar
Joined: 2008-11-04
4489 posts
pbreit - 13 July 2010 05:28 PM

Wan, your solution works with no side effects?

Never say never, but until now I haven’t seen any.

If you come across them (I can’t test every possible situation), let me know, and I’ll fix it.

 Signature 

Me: WanWizard.eu | My company: Exite | Datamapper: DataMapper ORM <= LOOKING FOR A NEW MAINTAINER!

 
Posted: 24 July 2010 05:14 AM   [ # 30 ]   [ Rating: 0 ]
Joined: 2009-11-26
237 posts

Wan, before going with your fix i have a question:
My site is using segments. Will this fix allow the segments work properly? I mean I need GET and segments working together.

 
2 of 10
2