Adds method_exists check to Model->__call() avoiding recursion issues for non-existent methods

[michaelward82]

Re: Issue #74

I'll try to find some time to add a regression test for this.

[treffynnon]

You'll need to get a newer version of PHPUnit to run the tests. They require 3.7+. I use the phar file downloadable from the PHPUnit site as it is much easier than mucking around with the packages in Linux repositories or from PEAR.

[michaelward82]

Same thing with 3.7:

PHPUnit 3.7.26 by Sebastian Bergmann.

Configuration read from /web/other/paris2/phpunit.xml

PHP Fatal error:  Call to a member function getAttribute() on a non-object in /web/other/paris2/test/idiorm.php on
line 327
PHP Stack trace:
PHP   1. {main}() /usr/local/bin/phpunit:0
PHP   2. PHPUnit_TextUI_Command::main() /usr/local/bin/phpunit:612
PHP   3. PHPUnit_TextUI_Command->run() phar:///usr/local/bin/phpunit/phpunit/TextUI/Command.php:129
PHP   4. PHPUnit_TextUI_TestRunner->doRun() phar:///usr/local/bin/phpunit/phpunit/TextUI/Command.php:176
PHP   5. PHPUnit_Framework_TestSuite->run() phar:///usr/local/bin/phpunit/phpunit/TextUI/TestRunner.php:349
PHP   6. PHPUnit_Framework_TestSuite->run() phar:///usr/local/bin/phpunit/phpunit/Framework/TestSuite.php:705
PHP   7. PHPUnit_Framework_TestSuite->runTest() phar:///usr/local/bin/phpunit/phpunit/Framework/TestSuite.php:745
PHP   8. PHPUnit_Framework_TestCase->run() phar:///usr/local/bin/phpunit/phpunit/Framework/TestSuite.php:775
PHP   9. PHPUnit_Framework_TestResult->run() phar:///usr/local/bin/phpunit/phpunit/Framework/TestCase.php:783
PHP  10. PHPUnit_Framework_TestCase->runBare() phar:///usr/local/bin/phpunit/phpunit/Framework/TestResult.php:648
PHP  11. ModelPrefixingTest->tearDown() phar:///usr/local/bin/phpunit/phpunit/Framework/TestCase.php:870
PHP  12. ORM::set_db() /web/other/paris2/test/ModelPrefixingTest.php:17
PHP  13. ORM::_setup_identifier_quote_character() /web/other/paris2/test/idiorm.php:282
PHP  14. ORM::_detect_identifier_quote_character() /web/other/paris2/test/idiorm.php:303

When I hit the error producing line 327 of idiorm.php, I get the following evaluation in debug:

$_db[$connection_name] = null

which causes this call to fail:

self::$_db[$connection_name]->getAttribute(PDO::ATTR_DRIVER_NAME)

It seems a null db is passed in at step 11 as part of the tearDown function, but said db is required to not be null at a later point.

In case it is relevant, the machine is running Ubuntu 13.04, with PHP 5.4.9-4.

[treffynnon]

That is really weird. I am running the tests locally just fine and Travis-CI is also running the tests successfully.

simon@Treffynnon:~/paris [master] $ php ../phpunit.phar 
PHPUnit 3.7.24 by Sebastian Bergmann.

Configuration read from /home/simon/paris/phpunit.xml

.....................................................

Time: 78 ms, Memory: 4.50Mb

OK (53 tests, 61 assertions)

[michaelward82]

Bizarre. Must be something funky with my setup.

I'll log on to my home mac os x install and try it from there!

[michaelward82]

And from my mac it works fine! Has to be an env problem - I'll just have to do the work from the mac!

PHPUnit 3.7.26 by Sebastian Bergmann.

Configuration read from /Users/michael/dev/paris/phpunit.xml

.....................................................

Time: 183 ms, Memory: 4.50Mb

OK (53 tests, 61 assertions)

[michaelward82]

I tried adding the following test to ParisTest.php, but when this is present I get a segmentation fault.

public function testInvalidFunctionCallDoesNotRecurse() {
    $result = Model::factory('Simple');
    $result = $result->findNone();
    $this->assertEquals(false, $result);
}

Clearly I am looking for false as the return value from the non-existent method call.

Any suggestions?

[michaelward82]

I think I've got the regression test working correctly, though I do get a segmentation fault rather than an assertion error when i remove the bug fix from the code and retest.

[treffynnon]

I would suggest that it should throw an exception rather than return false:

throw new Exception("Method $name() does not exist in class " . get_class($this));

[treffynnon]

The reason why the original test was not working is because you are not getting an instance of Model when calling Model::factory(), but an instance of ORMWrapper. ORMWrapper extends Idiorm's ORM class, which has an __call() method in it. That __call() method in there does not have any method_exists() protection. As soon as I add in a similar method_exists() protection your code works as expected with your original regression test.

So this pull request needs to be accompanied with change/pull request in Idiorm itself as well.

[michaelward82]

Yeah, I realised that in the end :)

The protection can exist independently in the Model class, though we obviously need to place it in any class that uses this type of functionality.

Is it just the ORM class that needs the fix or will I find it elsewhere?

I'll sort out the code and send a pull request this evening.

Is everything in order with this patch now?

[treffynnon]

Yes, looks good. Although I would like to have the original regression test included again as well.

It should just be ORM that needs the patch I think.

[michaelward82]

Ok, I'll add a test for idiorm when I fix ORM, and will restore the previous regression test to the Paris test suite when the idiorm work is done.

[michaelward82]

Do we need a custom exception class to satisfy the old version of PHPUnit being used for PHP 5.2 testing by Travis?

See sebastianbergmann/phpunit#454 for further details.

[treffynnon]

Aside from that one file mentioned above this looks good. Thanks!

[michaelward82]

Sorry, it was an unintended PHPStorm project file that shouldn't have been checked in.

[shdwp]

I think you just need to trigger fatal error, just like PHP without __call does.

[treffynnon]

I don't like throwing uncatchable fatal errors from within a library as I feel this is something that should be an implementers decision. They can ignore it, allow the exception to halt proceedings or handle the exception in another way.

[amerker]

I also get infinite recursions when trying to access nullable DB fields from Twig, via {{ object.field }}, that actually have NULL as value. When accessing the corresponding method, breakage ensues. This is when building the object using find_one(), or find_many() plus Twig loop. It's interesting that the Twig test "is defined" always returns true regardless of whether the field is NULL or has data.

When using find_array() I can access the NULL field from Twig as expected.

[gwn]

Hey @treffynnon . What is the fate of this pull request? Will it be merged? Is there anything that is still missing?

[treffynnon]

@Cal127 it is earmarked for the 1.5.0 release milestone which is currently scheduled for the end of January. I have recently assigned all outstanding pull requests to a milestone.

[gwn]

Thanks

[treffynnon]

Merged in develop