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

Already on GitHub? Sign in to your account

Make Model::find('first') always return an array. #917

Merged
merged 1 commit into from Oct 26, 2012

Conversation

Projects
None yet
8 participants
Contributor

bar commented Oct 25, 2012

Model::find('first') should honor its return value. This way Model::find() can be consistent returning values (and honoring its return value) too.

Contributor

bar commented Oct 25, 2012

Guided by the documentation, I consider this a bug, so this is targeted to master.

When calling Model::find() and passing the values to Hash::extract() later, despite working, there is always a signature warning because the find is not consistent when it finds no rows. At least Model::find('all') and Model::find('first') should be consistent as they are the most used find methods.

Owner

markstory commented Oct 26, 2012

I think this change is pretty reasonable. Does anyone else have an opinion?

Member

ADmad commented Oct 26, 2012

👍

markstory added a commit that referenced this pull request Oct 26, 2012

Merge pull request #917 from bar/master-find
Make Model::find('first') always return an array.

@markstory markstory merged commit a0665fe into cakephp:master Oct 26, 2012

Member

ceeram commented Oct 26, 2012

Caused a failing test, fixed here: 39dcb80

Member

rchavik commented Nov 3, 2012

IMO, this should have gone into 2.3 instead.

Owner

markstory commented Nov 3, 2012

@rchavik Why? It seems like the behavior isn't following the documentation.

Member

rchavik commented Nov 3, 2012

It's BC breaker. i chased my own tail for a few hours trying to figure out why my simple Coding Style fixes suddenly failed.

And when code and doc differs, code should win I think.

Owner

markstory commented Nov 3, 2012

I guess you were updating from $f == false to $f === false?

Member

rchavik commented Nov 3, 2012

No, just simple CS changes, eg: removing extra space, adding space between . operators, removing indents.
No significant logic changes was made:

croogo/croogo@b1e7d0f...53f3fdd

The failure result was totally unrelated and confusing:

~/work/personal/deploy/croogo (1.5)(v1.4.3-176-gbbbbb6e)
rachman@fingolfin $ Console/cake test app AllTests --stderr

Welcome to CakePHP v2.2.3 Console
---------------------------------------------------------------
App : croogo
Path: /home/rachman/work/personal/deploy/croogo/
---------------------------------------------------------------
CakePHP Test Shell
---------------------------------------------------------------
..................................................E............  63 / 204 ( 30%)
............................................................... 126 / 204 ( 61%)
...................................................E........... 189 / 204 ( 92%)
...............

Time: 01:50, Memory: 148.50Mb

There were 2 errors:

1) ExtShellTest::testPlugin
Undefined index: Link

/mnt/user-data/users/rachman/work/personal/deploy/croogo/Plugin/Example/Config/ExampleActivation.php:85
/mnt/user-data/users/rachman/work/personal/deploy/croogo/Plugin/Extensions/Lib/CroogoPlugin.php:416
/mnt/user-data/users/rachman/work/personal/deploy/croogo/Console/Command/ExtShell.php:181
/mnt/user-data/users/rachman/work/personal/deploy/croogo/Console/Command/ExtShell.php:117
/mnt/user-data/users/rachman/work/personal/deploy/croogo/Test/Case/Console/Command/ExtShellTest.php:83
/mnt/user-data/users/rachman/work/core/cake_2.0/lib/Cake/TestSuite/CakeTestCase.php:78
/mnt/user-data/users/rachman/work/core/cake_2.0/lib/Cake/TestSuite/CakeTestRunner.php:59
/mnt/user-data/users/rachman/work/core/cake_2.0/lib/Cake/TestSuite/CakeTestSuiteCommand.php:111
/mnt/user-data/users/rachman/work/core/cake_2.0/lib/Cake/Console/Command/TestShell.php:274
/mnt/user-data/users/rachman/work/core/cake_2.0/lib/Cake/Console/Command/TestShell.php:259
/mnt/user-data/users/rachman/work/core/cake_2.0/lib/Cake/Console/Shell.php:393
/mnt/user-data/users/rachman/work/core/cake_2.0/lib/Cake/Console/ShellDispatcher.php:201
/mnt/user-data/users/rachman/work/core/cake_2.0/lib/Cake/Console/ShellDispatcher.php:69

2) TranslateControllerTest::testAdminEditWithBogusLanguage
Undefined index: Language

/mnt/user-data/users/rachman/work/personal/deploy/croogo/Plugin/Translate/Controller/TranslateController.php:143
/mnt/user-data/users/rachman/work/core/cake_2.0/lib/Cake/Controller/Controller.php:485
/mnt/user-data/users/rachman/work/core/cake_2.0/lib/Cake/Routing/Dispatcher.php:186
/mnt/user-data/users/rachman/work/core/cake_2.0/lib/Cake/Routing/Dispatcher.php:161
/mnt/user-data/users/rachman/work/core/cake_2.0/lib/Cake/TestSuite/ControllerTestCase.php:271
/mnt/user-data/users/rachman/work/core/cake_2.0/lib/Cake/TestSuite/ControllerTestCase.php:189
/mnt/user-data/users/rachman/work/personal/deploy/croogo/Plugin/Translate/Test/Case/Controller/TranslateControllerTest.php:63
/mnt/user-data/users/rachman/work/personal/deploy/croogo/Plugin/Translate/Test/Case/Controller/TranslateControllerTest.php:63
/mnt/user-data/users/rachman/work/core/cake_2.0/lib/Cake/TestSuite/CakeTestCase.php:78
/mnt/user-data/users/rachman/work/core/cake_2.0/lib/Cake/TestSuite/CakeTestRunner.php:59
/mnt/user-data/users/rachman/work/core/cake_2.0/lib/Cake/TestSuite/CakeTestSuiteCommand.php:111
/mnt/user-data/users/rachman/work/core/cake_2.0/lib/Cake/Console/Command/TestShell.php:274
/mnt/user-data/users/rachman/work/core/cake_2.0/lib/Cake/Console/Command/TestShell.php:259
/mnt/user-data/users/rachman/work/core/cake_2.0/lib/Cake/Console/Shell.php:393
/mnt/user-data/users/rachman/work/core/cake_2.0/lib/Cake/Console/ShellDispatcher.php:201
/mnt/user-data/users/rachman/work/core/cake_2.0/lib/Cake/Console/ShellDispatcher.php:69

FAILURES!
Tests: 204, Assertions: 621, Errors: 2.

That run was against: master -> 2.2.3-81-gc60237e

The same test suite passes against 2.2.3.

Member

ceeram commented Nov 3, 2012

If code and docs differ, code should not always win. If the code not always
act as the docs describe what it is intended to do/return it can be
considered a bug or incorrect docs, in this case i would consider it a bug
Op 3 nov. 2012 15:07 schreef "Rachman Chavik" notifications@github.com
het volgende:

No, just simple CS changes, eg: removing extra space, adding space between
. operators, removing indents.
No significant logic changes was made:

croogo/croogo@b1e7d0f...53f3fdd

The failure result was totally unrelated and confusing:

~/work/personal/deploy/croogo (1.5)(v1.4.3-176-gbbbbb6e)
rachman@fingolfin $ Console/cake test app AllTests --stderr

Welcome to CakePHP v2.2.3 Console

App : croogo

Path: /home/rachman/work/personal/deploy/croogo/

CakePHP Test Shell

..................................................E............ 63 / 204 ( 30%)
............................................................... 126 / 204 ( 61%)
...................................................E........... 189 / 204 ( 92%)
...............

Time: 01:50, Memory: 148.50Mb

There were 2 errors:

  1. ExtShellTest::testPlugin
    Undefined index: Link

/mnt/user-data/users/rachman/work/personal/deploy/croogo/Plugin/Example/Config/ExampleActivation.php:85
/mnt/user-data/users/rachman/work/personal/deploy/croogo/Plugin/Extensions/Lib/CroogoPlugin.php:416
/mnt/user-data/users/rachman/work/personal/deploy/croogo/Console/Command/ExtShell.php:181
/mnt/user-data/users/rachman/work/personal/deploy/croogo/Console/Command/ExtShell.php:117
/mnt/user-data/users/rachman/work/personal/deploy/croogo/Test/Case/Console/Command/ExtShellTest.php:83
/mnt/user-data/users/rachman/work/core/cake_2.0/lib/Cake/TestSuite/CakeTestCase.php:78
/mnt/user-data/users/rachman/work/core/cake_2.0/lib/Cake/TestSuite/CakeTestRunner.php:59
/mnt/user-data/users/rachman/work/core/cake_2.0/lib/Cake/TestSuite/CakeTestSuiteCommand.php:111
/mnt/user-data/users/rachman/work/core/cake_2.0/lib/Cake/Console/Command/TestShell.php:274
/mnt/user-data/users/rachman/work/core/cake_2.0/lib/Cake/Console/Command/TestShell.php:259
/mnt/user-data/users/rachman/work/core/cake_2.0/lib/Cake/Console/Shell.php:393
/mnt/user-data/users/rachman/work/core/cake_2.0/lib/Cake/Console/ShellDispatcher.php:201
/mnt/user-data/users/rachman/work/core/cake_2.0/lib/Cake/Console/ShellDispatcher.php:69

  1. TranslateControllerTest::testAdminEditWithBogusLanguage
    Undefined index: Language

/mnt/user-data/users/rachman/work/personal/deploy/croogo/Plugin/Translate/Controller/TranslateController.php:143
/mnt/user-data/users/rachman/work/core/cake_2.0/lib/Cake/Controller/Controller.php:485
/mnt/user-data/users/rachman/work/core/cake_2.0/lib/Cake/Routing/Dispatcher.php:186
/mnt/user-data/users/rachman/work/core/cake_2.0/lib/Cake/Routing/Dispatcher.php:161
/mnt/user-data/users/rachman/work/core/cake_2.0/lib/Cake/TestSuite/ControllerTestCase.php:271
/mnt/user-data/users/rachman/work/core/cake_2.0/lib/Cake/TestSuite/ControllerTestCase.php:189
/mnt/user-data/users/rachman/work/personal/deploy/croogo/Plugin/Translate/Test/Case/Controller/TranslateControllerTest.php:63
/mnt/user-data/users/rachman/work/personal/deploy/croogo/Plugin/Translate/Test/Case/Controller/TranslateControllerTest.php:63
/mnt/user-data/users/rachman/work/core/cake_2.0/lib/Cake/TestSuite/CakeTestCase.php:78
/mnt/user-data/users/rachman/work/core/cake_2.0/lib/Cake/TestSuite/CakeTestRunner.php:59
/mnt/user-data/users/rachman/work/core/cake_2.0/lib/Cake/TestSuite/CakeTestSuiteCommand.php:111
/mnt/user-data/users/rachman/work/core/cake_2.0/lib/Cake/Console/Command/TestShell.php:274
/mnt/user-data/users/rachman/work/core/cake_2.0/lib/Cake/Console/Command/TestShell.php:259
/mnt/user-data/users/rachman/work/core/cake_2.0/lib/Cake/Console/Shell.php:393
/mnt/user-data/users/rachman/work/core/cake_2.0/lib/Cake/Console/ShellDispatcher.php:201
/mnt/user-data/users/rachman/work/core/cake_2.0/lib/Cake/Console/ShellDispatcher.php:69

FAILURES!
Tests: 204, Assertions: 621, Errors: 2.

That run was against: master -> 2.2.3-81-gc60237e

The same test suite passes against 2.2.3.


Reply to this email directly or view it on GitHubhttps://github.com/cakephp/cakephp/pull/917#issuecomment-10039666.

Member

rchavik commented Nov 3, 2012

The existence of some tests that assumes that the result is false suggest that the code was 'correct' at some point.

Contributor

bar commented Nov 3, 2012

@rchavik albeit the problem, I still don't understand how is this related to the error you are seeing. If not a type check as @markstory suggests, could you find the offending lines?

Member

rchavik commented Nov 4, 2012

@bar that's why i said 'totally unrelated and confusing'. I wouldn't have guessed this caused it without bisect telling me. Rerunning the same test with the 'return false' in _findFirst() fixes the problem.

This shouldn't happen for a minor release.

Contributor

bar commented Nov 4, 2012

@rchavik the thing is, you should be expecting an array, from the start. That's why this is a bugfix.

From what I see, the code is accesing an array index directly without even checking it first. I didn't pay much attention but it seems some afterFind() is messing with the return value, can that be possible?

Member

rchavik commented Nov 4, 2012

@bar, why? because the description in the @return docblock? How can we be sure that's the correct/expected behavior? I'm pretty sure there are few instances in the current codebase where the docblock differs with actual behavior. So, I would trust what the current code tells me.

I was going to revert this but now I won't anymore since it does not apply cleanly.

The false value has been there from from 1.2 days, there could be a lot of apps out there that already depends on false being the expected return value. So, can you update the migration guide about this behavior change since it is a compatibility breaker.

Contributor

bar commented Nov 4, 2012

@rchavik Of course there is, in fact I have documented some of them myself. I have the same habits BTW, inspecting the code before reading docblocks at least. When they are found they are fixed, sometimes the docs are old and does not reflect what the intended behavior is.

Apart from that, I'm just trying to help you find the problem in Croogo, and myself trying to understand it.

I also think, being in master (2.2.3 right now) or 2.3.0 is not going to make a big difference, it is a matter of days (weeks?¿) I think. And I do understand it is a versioning issue what you suggest.

Member

ADmad commented Nov 4, 2012

So fails @rchavik is getting are not actually due to this change but due to the way how Controller::redirect() is handled in tests. In tests since Controller::redirect() is mocked execution continues even after a redirect which would not happen in actual usage. So regardless of find('first') returning array() or false this check succeeds and redirect is executed. But in test case execution continues to the following lines of code and triggering a notice at this line.

He didn't get an error earlier because php doesn't throw a notice if you try to use index of a false value. http://codepad.org/whzBhhc7

Edit: It's actually the Controller::_stop() that get's mocked (for obvious reasons) not Controller::redirect().

Contributor

bar commented Nov 4, 2012

Thanks for the info @ADmad, I'm seeing that right here https://github.com/cakephp/cakephp/blob/master/lib/Cake/TestSuite/ControllerTestCase.php#L324.

So @rchavik, calling redirect like return $this->redirect() inside that kind of test should be enough I think.

Member

rchavik commented Nov 5, 2012

Regardless of the fix in croogo, this change modifies known and existing behavior and breaks backward compatibility. Please help to document it in the migration guide.

rchavik added a commit to rchavik/croogo that referenced this pull request Nov 5, 2012

rchavik added a commit to rchavik/croogo that referenced this pull request Nov 5, 2012

rchavik added a commit to rchavik/croogo that referenced this pull request Dec 27, 2012

Contributor

func0der commented on c741f60 Jan 31, 2013

$result = false;
$result['aa'] <-- would not produce any PHP Notice.

$result = array();
$result['aa'] <-- would produce a PHP Notice.

This is just an example of the advantage of false which should not occure anyway, because you'd check with isset().

I don't get this change. Why does $Model->find('first') should return an array instead of false?

Owner

lorenzo replied Jan 31, 2013

I think it is pretty obvios that $result['aa'] should return a notice if you access something that does not exist, I don't get why would you consider that something desirable

Owner

markstory replied Jan 31, 2013

The thinking was that it was the only find type to return false. find(count) clearly will return an int at all times. But find(all, list, neighbors) all return arrays even on failure.

Member

dereuromark replied Jan 31, 2013

@ func0der: You do not check with isset() here but with !emtpy(). isset() would only work for NULL return values.

You can also just check the result directly. So using an empty array you have the exact same behavior as false if you do:

$result = $this->Model->find('first');
if ($result) {
    $value = $result['Model']['value'];
} else {
    // no record found - do not access the array
}

If there is no result you should not try to access any keys/values in that array.

Contributor

func0der replied Feb 1, 2013

@dereuromark You used the wrong name :P
I meant to use "isset($result['aa'])" to check for an index in an array to be set. Isset() won't work on an empty array, that's true.
I tested your code before posting my initial comment, and recognized that, too. It was one of my main concernes about this change. But you are right, there is no real difference.

@markstory Ahh. Okay. Thank you for that information. I thought find('all') returns also false, but I was wrong there as it seems. Considering that you are trying to unify all the finds shouldn't read() be changed to this behavior, too? The return comment on read() is wrong at the moment: " * @return array Array of database fields, or false if not found". It does return false only if the id is NULL or FALSE. If there were no entries in the database for that special id it woud return an empty array, because it is just a wrapper method for find('first') with primary key as condition.

@lorenzo You are right, PHP should return something. But if the content of the variable is actually FALSE and not an array it would not trigger any notice messages if the requested index is not present. This saves just some lines of code here and there and is not really best or even good practice. I just mentioned it, because maybe some applications are relying on that fact. My core question was: Why the change. Which markstory answered already ;)

Owner

markstory replied Feb 1, 2013

read() is a warty method on Model. It will probably be removed in 3.x as its inconsistent and bad.

Contributor

bar replied Feb 1, 2013

@func0der to late here but the rationale was #917

Contributor

func0der replied Feb 1, 2013

Yeah. You are right. I used it so much before is recognized it kills my whole application because model states are saved in classregistry.

Whatever. Thank all of you for your explanation. :)

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