Type checks #846

Closed
wants to merge 11 commits into
from

4 participants

@dogmatic69

converting $foo == null / $foo == false to !$foo

converting $foo == "" / $foo == 0 to !$foo (and a few $foo === 0)

converting if($foo == true) to if($foo)

strict type checks and removing some duplicate count() calls

@ADmad ADmad commented on an outdated diff Sep 14, 2012
lib/Cake/Console/Command/ApiShell.php
$file = Inflector::underscore($this->args[1]);
$class = Inflector::camelize($this->args[1]);
+ } elseif($count) {
@ADmad
CakePHP member
ADmad added a line comment Sep 14, 2012

space after elseif

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ADmad ADmad commented on an outdated diff Sep 14, 2012
lib/Cake/Model/Datasource/DboSource.php
@@ -1053,7 +1053,7 @@ public function read(Model $model, $queryData = array(), $recursive = null) {
if ($model->recursive == -1) {
$_associations = array();
- } elseif ($model->recursive == 0) {
+ } elseif (!$model->recursive) {
@ADmad
CakePHP member
ADmad added a line comment Sep 14, 2012

This I would rather change to $model->recursive === 0 since only numeric values are meaningful for recursive.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@markstory markstory commented on the diff Sep 14, 2012
lib/Cake/Console/Command/Task/DbConfigTask.php
@@ -170,7 +170,7 @@ protected function _interactive() {
$schema = '';
if ($datasource == 'postgres') {
- while ($schema == '') {
+ while (!$schema) {
@markstory
CakePHP member
markstory added a line comment Sep 14, 2012

This is a minor change in behaviour as '0' is no longer an acceptable value. However, in the above cases I don't think its relevant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@markstory markstory commented on an outdated diff Sep 14, 2012
lib/Cake/Controller/Controller.php
@@ -454,7 +454,7 @@ public function setRequest(CakeRequest $request) {
$this->passedArgs = array_merge($request->params['pass'], $request->params['named']);
}
- if (array_key_exists('return', $request->params) && $request->params['return'] == 1) {
+ if (!empty($request->params['return']) && $request->params['return']) {
@markstory
CakePHP member
markstory added a line comment Sep 14, 2012

This is a slight change too as 'return' => 'bears' will now pass where it used to fail.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@markstory markstory commented on the diff Sep 14, 2012
lib/Cake/View/Helper.php
@@ -526,12 +526,11 @@ public function setEntity($entity, $setScope = false) {
$isHabtm = (
isset($this->fieldset[$this->_modelScope]['fields'][$parts[0]]['type']) &&
- $this->fieldset[$this->_modelScope]['fields'][$parts[0]]['type'] === 'multiple' &&
- $count == 1
@markstory
CakePHP member
markstory added a line comment Sep 14, 2012

Isn't this a behavior change as well, the count could be > 1 now and pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dogmatic69 dogmatic69 commented on the diff Sep 14, 2012
lib/Cake/View/Helper.php
);
// habtm models are special
- if ($count == 1 && $isHabtm) {
+ if ($count === 1 && $isHabtm) {
@dogmatic69
dogmatic69 added a line comment Sep 14, 2012

@markstory There was a check for the $count here again

@markstory
CakePHP member
markstory added a line comment Sep 15, 2012

Ah I missed that thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dereuromark
CakePHP member

seems like you have been inspired by my ticket http://cakephp.lighthouseapp.com/projects/42648/tickets/2101-checking-on-null-values-not-strict-partly-incorrect
but yours is way more detailed and correct. great work so far.

@dogmatic69

Thanks, just from a comment @markstory made on a previous commit and figured there would be some more.

@dereuromark
CakePHP member

@dogmatic69 There are also some != issues.

Model.php read() - line 1502:

if ($id != null) {

should be

if ($id) {

for instance

@bar

Shouldn't it be ($model->recursive === 0) without the ! ?¿

CakePHP member

yes, it should. why not leaving it as it was here? Since users could use "0" (even if thats not really pretty) as a string here and I dont see any casting going on.

@dogmatic69

anything else needed here?

@markstory
CakePHP member

The changes look good to me. However, it doesn't look like the changes will merge without a conflict. Could you fix that?

@dogmatic69 dogmatic69 Merge branch '2.3' into type-checks
Conflicts:
	lib/Cake/Console/Command/Task/ModelTask.php
	lib/Cake/Controller/Component/RequestHandlerComponent.php
	lib/Cake/Model/Datasource/Database/Mysql.php
	lib/Cake/Utility/CakeNumber.php
408e619
@dogmatic69

There was 4 conflicts,

lib/Cake/Console/Command/Task/ModelTask.php
lib/Cake/Controller/Component/RequestHandlerComponent.php
lib/Cake/Model/Datasource/Database/Mysql.php
lib/Cake/Utility/CakeNumber.php

Re ran the tests for those files and all passing.

@dereuromark
CakePHP member

I think the changes are really worth it!

PS: got (well, or the other Mark to be exact) some more work for you, @dogmatic: 57f81da

@markstory
CakePHP member

Closing as this doesn't apply anymore. If you rebase the changes again I'll try to merge them in right away.

@markstory markstory closed this Oct 23, 2012
@dogmatic69 dogmatic69 referenced this pull request Oct 24, 2012
Merged

Type checks #911

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment