CodeIgniter 3.0 Proposed Change: NULL v FALSE

[eluser]Phil Sturgeon[/eluser]
CodeIgniter has always had a history of being backwards compatible with releases and this is what makes it one of the most used frameworks around. Some strive to constantly be the best by rewriting themselves and breaking their API, some even do total recodes. None of these approaches are neccessairily wrong, but they get a different crowd of developers.

Recently the biggest change between 1.7.x and 2.x was adding in a prefix to some classes, and between 2.0.x and 2.1.x was even less than that. It's always been easy, and the fact that I can bring a codebase that is 4 years old up to date in no time is a blessing.

It's because of this that some changes are hard to decide on, even if they would be "for the best". This proposed change is that the following methods should return NULL instead of FALSE:

Code:

$this->input->get();
$this->input->get_post();
$this->input->post();
$this->input->cookie();
$this->input->server();
$this->input->post();

$this->session->userdata();
$this->session->flashdata();

Why do this? Does it matter? Well yes, while at first glance it appears trivial it has two benefit; When these items do not know what a value is (because none was provided) they should say NULL. By instead providing FALSE we are actually getting a value, and so when we try to save this in a SQL database the PHP drivers convert FALSE to 0, and we end up with 0 in our database! That is pretty annoying.

Now we can do checks for each of these every time, but they are a pain in the backside.

Code:

// NULL default in PHP 5.3

$foo = $this->input->post('foo') ?: NULL;

// NULL default in PHP 5.2

$foo = $this->input->post('foo') ? $this->input->post('foo') : NULL;

// NULL default proposal in CodeIgniter 3.0

$foo = $this->input->post('foo');

Compatibility

If this change were to be made, you may well find that the majority of your code is absolutely fine.

Example:

Code:

if ($this->input->post('neverset') == FALSE)
if ( ! $this->input->post('neverset'))

Both of those statements will still work just fine, as FALSE == NULL. You would only notice a problem if you had used:

Code:

if ($this->input->post('neverset') === FALSE)

That would be fail to match, as FALSE !== NULL.

Thoughts

I'm writing this post not to tell you what I plan on doing, as that's not how us Engineer types roll! We are part of the community and want to work WITH you guys for the best. I put out a Tweet and 90% of the feedback was positive, with 5% sounding confused about the benefits (explained better above now) and 5% giving odd answers I can barely associate with any form of reason.

I would appreciate feedback on this, so we can see how to move forwards, or I can throw the idea out the window. I'd rather not have to do that, as NULL is the correct way to do things. At some point we will need to do it correctly, and now rather in a year would be a much better idea.

[eluser]JonoB[/eluser]
Yes from me. I'd much rather things are done the 'right' way than leaving bad code as-is.

[eluser]William Rufino[/eluser]
Yes! We should aways try to make it better!

[eluser]n0xie[/eluser]
It is a shame that you can't use the second param for 'default' value like most of the 'other' frameworks do

Maybe you can add a wrapper helper class. Something we do:

Code:

// if $this->input->get('bar') === FALSE, we return NULL
$foo = $this->request->get('bar');
// if $foo is not set or empty, we return 'baz'
$foo = $this->request->get('bar', 'baz');
// the same as above but with a POST example
$image_link = $this->request->post('gravatar', 'http://placehold.it/50x50');

This way you will keep the old API intact (i.e. don't break backwards compatibility) and have a reason for people to switch to the new API (benefit of filling in defaults). The currest XSS filter as 2nd param is imho too tightly coupled with just retrieving a param.

[eluser]daparky[/eluser]
[quote author="n0xie" date="1335270625"]It is a shame that you can't use the second param for 'default' value like most of the 'other' frameworks do

Maybe you can add a wrapper helper class. Something we do:

Code:

// if $this->input->get('bar') === FALSE, we return NULL
$foo = $this->request->get('bar');
// if $foo is not set or empty, we return 'baz'
$foo = $this->request->get('bar', 'baz');
// the same as above but with a POST example
$image_link = $this->request->post('gravatar', 'http://placehold.it/50x50');

This way you will keep the old API intact (i.e. don't break backwards compatibility) and have a reason for people to switch to the new API (benefit of filling in defaults). The currest XSS filter as 2nd param is imho too tightly coupled with just retrieving a param.[/quote]

I really like this idea.

[eluser]Phil Sturgeon[/eluser]
n0xie: I considered adding a second option but most of these methods have second parameters already.

I think adding in a new class for all this is just going to confuse matters, and people will keep using the old system not bothering to use the new, then when we deprecate and eventually remove the old method users have the problem they have right now, but a year down the line.

[eluser]Spir[/eluser]
+1 on returning NULL rather than FALSE
[quote author="n0xie" date="1335270625"]It is a shame that you can't use the second param for 'default' value like most of the 'other' frameworks do[/quote]+1

[eluser]n0xie[/eluser]
I think depreciating functionality and in the long run removing it, will make dev(s) responsible for the problem, while changing an API interface will make the code maintainer(s) responsible. Depreciating means 'use at your own risk, if you blow something up, tough luck, we warned you', while changing an interface means 'dear dev, f*** y**, sincerely, the maintainer'. At least that's how I see it liberally being used in OSS. Not saying that is the intention, but I have seen many projects that changed the API significantly without informing their userbase (enough) and getting a lot of hate for it.

That being said, the change here is minor, so I doubt any major issues will arise.

[eluser]Phil Sturgeon[/eluser]
I am against the "second param as default" approach as that was originally coded in by frameworks like Kohana to avoid the lack of conditional assignment operators in PHP. They did it back in 5.1 and it was still useful in 5.2, but should we be starting to code things now that will be pointless by PHP 5.3?

Code:

$this->input->post('foo', 'default');
// or the 5.3 way
$this->input->post('foo') ?: 'default';

I don't want to start coding things now that will need to be thrown away, especially if the second parameter is already used by something else and will likely need to be used for something more useful in the future.

[eluser]Phil Sturgeon[/eluser]
[quote author="n0xie" date="1335271522"]...I have seen many projects that changed the API significantly without informing their userbase (enough) and getting a lot of hate for it.

That being said, the change here is minor, so I doubt any major issues will arise.[/quote]

Absolutely and CodeIgniter will never be those guys, but we do have to make changes now and then and a x.0 release is the time to do it.

Some frameworks have put in place major logical rewrites on a 0.x release (which is madness) and others have entirely rewritten everything to the point where you might as well give it a different name.

This is a simple change, that will involve probably an hour of find and replace. If it's in the changelog, the upgrade guide and blog it won't be anything that a developer could miss. A developer should ALWAYS read those before upgrading, and be using a local environment to check their code works before deployment, so this shouldn't be seen as a large problem, or cause any major problems with anyones app.