Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't assume connection is available in ORM::save #137

Closed
wants to merge 1 commit into from

Conversation

collegeman
Copy link

If ORM::save is called before any other querying method and logging is enabled, logging will fail.

If `ORM::save` is called before any other querying method and logging is enabled, logging will fail.
@treffynnon
Copy link
Collaborator

Do you have a regression test for this?

@collegeman
Copy link
Author

I'm using idiorm and paris with SlimFramework.

By default, SlimFramework turns notice-level errors into Exceptions,
without which I probably would never have known about this (because it only
affects logging, and then it seems it only affects logging when the
underlying query includes parameters).

My code looks something like this:

$factory = Model::factory('MyClass');
$instance = $factory->create();
$instance->property = 'value';
$instance->save();

If this code is executed without any preceding query execution and with
logging enabled, I get this Exception:

Undefined index: 'default'

thrown by this line in ORM::_log_query:

$parameters = array_map(array(self::$_db[$connection_name], 'quote'),
$parameters);

Because, at least in my case, self::$_db['default'] doesn't exist yet.

I dug into the code and what I found was that generally ORM::_setup_db is
called immediately before calling ORM::_execute. Fulfilling that pattern
inside ORM::save fixed my problem.

Having now explained this, I see that the same thing is probably possible
in several other functions:

ORM::delete
ORM::delete_many
ORM::_run

If you agree to this fix, I'd be happy to amend my pull request to
remediate this issue in those other functions.

Cheers!


Aaron Collegeman
[email protected]

On Tue, Jul 16, 2013 at 7:46 AM, Simon Holywell [email protected]:

Do you have a regression test for this?


Reply to this email directly or view it on GitHubhttps://github.com//pull/137#issuecomment-21036704
.

@treffynnon
Copy link
Collaborator

Have you definitely got the latest master versions of both Paris and Idiorm? I would have thought that this was unnecessary as there is a call to for_table() (https://github.com/j4mie/paris/blob/master/paris.php#L94) in factory() (https://github.com/j4mie/paris/blob/master/paris.php#L253) which calls _setup_db(). So this should not actually be happening.

@collegeman
Copy link
Author

Yes, I have the latest. Just confirmed. Only difference between my idiorm
and master is the addition of my call to ORM::_setup_db


Aaron Collegeman
[email protected]

On Tue, Jul 16, 2013 at 12:03 PM, Simon Holywell
[email protected]:

Have you definitely got the latest master versions of both Paris and
Idiorm? I would have thought that this was unnecessary as there is a call
to for_table() (https://github.com/j4mie/paris/blob/master/paris.php#L94)
in factory() (https://github.com/j4mie/paris/blob/master/paris.php#L253)
which calls _setup_db(). So this should not actually be happening.


Reply to this email directly or view it on GitHubhttps://github.com//pull/137#issuecomment-21052536
.

@ghost ghost assigned treffynnon Aug 14, 2013
@treffynnon
Copy link
Collaborator

I have attempted to replicate this issue and I cannot. Here is my code:

ini_set('error_reporting', E_ALL);
ini_set('show_errors', true);

require_once '../idiorm/idiorm.php';
require_once 'paris.php';

class CustomClass extends Model {

}

ORM::configure('logging', true);
ORM::configure('sqlite::memory:');

$factory = Model::factory('CustomClass');
$instance = $factory->create();
$instance->property = 'value';
$instance->save();

I am not seeing any errors or notices from this. I am removing it from the 1.4.0 milestone for now.

Do you have any further detail?

@treffynnon
Copy link
Collaborator

I am looking to release 1.4.0 in the coming week. If this is still causing an issue please update the ticket with more information. Thanks.

@collegeman
Copy link
Author

Thanks Simon! I don't have any further detail. The projects I was using
idiorm for are on the backburner, so I'm afraid I can't dig deeper right
now. The connection issue I was experiencing was consistent across both
projects, but that certainly doesn't eliminate the idea that I was doing
something wrong. Ship it! :)


Aaron Collegeman
[email protected]

On Tue, Sep 3, 2013 at 7:56 AM, Simon Holywell [email protected]:

I am looking to release 1.4.0 in the coming week. If this is still causing
an issue please update the ticket with more information. Thanks.


Reply to this email directly or view it on GitHubhttps://github.com//pull/137#issuecomment-23707071
.

@falmp
Copy link
Contributor

falmp commented Oct 1, 2013

Any chance you were using cached items? I've got into this problem while caching my objects (resources variables cannot be stored in the cache).

Taking the other places you mentioned, I have a little different version on a fork here: https://github.com/falmp/idiorm/tree/lazy-connection

How do I run the tests? I'm afraid I'm doing something wrong because I tried running phpunit against master before doing my changes but I got this error:

$ phpunit -c phpunit.xml 
PHPUnit 3.7.27 by Sebastian Bergmann.

Configuration read from /tmp/idiorm/phpunit.xml

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

Fatal error: Call to a member function getAttribute() on a non-object in /tmp/idiorm/idiorm.php on line 327

Call Stack:
    0.0023     307000   1. {main}() /home/falmp/bin/phpunit:0
    0.0628     729432   2. PHPUnit_TextUI_Command::main() /home/falmp/bin/phpunit:612
    0.0628     730056   3. PHPUnit_TextUI_Command->run() phar:///home/falmp/bin/phpunit/phpunit/TextUI/Command.php:129
    0.2924    4624256   4. PHPUnit_TextUI_TestRunner->doRun() phar:///home/falmp/bin/phpunit/phpunit/TextUI/Command.php:176
    0.3000    4915568   5. PHPUnit_Framework_TestSuite->run() phar:///home/falmp/bin/phpunit/phpunit/TextUI/TestRunner.php:349
    0.6080    7747480   6. PHPUnit_Framework_TestSuite->run() phar:///home/falmp/bin/phpunit/phpunit/Framework/TestSuite.php:705
    0.6081    7748080   7. PHPUnit_Framework_TestSuite->runTest() phar:///home/falmp/bin/phpunit/phpunit/Framework/TestSuite.php:745
    0.6081    7748760   8. PHPUnit_Framework_TestCase->run() phar:///home/falmp/bin/phpunit/phpunit/Framework/TestSuite.php:775
    0.6082    7749248   9. PHPUnit_Framework_TestResult->run() phar:///home/falmp/bin/phpunit/phpunit/Framework/TestCase.php:783
    0.6082    7751472  10. PHPUnit_Framework_TestCase->runBare() phar:///home/falmp/bin/phpunit/phpunit/Framework/TestResult.php:648
    0.6093    7811648  11. ConfigTest53->tearDown() phar:///home/falmp/bin/phpunit/phpunit/Framework/TestCase.php:870
    0.6093    7811776  12. ORM::set_db() /tmp/idiorm/test/ConfigTest53.php:18
    0.6094    7812104  13. ORM::_setup_identifier_quote_character() /tmp/idiorm/idiorm.php:282
    0.6094    7812168  14. ORM::_detect_identifier_quote_character() /tmp/idiorm/idiorm.php:303

@treffynnon
Copy link
Collaborator

@falmp We have recently had a similar issue with the Paris tests: j4mie/paris#75 (comment)

It all works fine on Travis and locally for me however. Is there anything special about your configuration?

@falmp
Copy link
Contributor

falmp commented Oct 1, 2013

Nope, except maybe it's the latest PHP version?

$ php -v
PHP 5.5.3 (cli) (built: Aug 23 2013 08:41:45) 
Copyright (c) 1997-2013 The PHP Group
Zend Engine v2.5.0, Copyright (c) 1998-2013 Zend Technologies
    with Xdebug v2.2.3, Copyright (c) 2002-2013, by Derick Rethans

$ php -m
[PHP Modules]
apc
apcu
Core
ctype
curl
date
dom
ereg
fileinfo
filter
gd
gettext
gmp
hash
iconv
imagick
intl
json
libxml
mbstring
mcrypt
memcache
mhash
mysql
mysqli
mysqlnd
openssl
pcntl
pcre
PDO
pdo_mysql
phalcon
Phar
posix
readline
redis
Reflection
session
SimpleXML
sockets
SPL
sqlite3
standard
tokenizer
xdebug
xml
xmlreader
xmlwriter
zip
zlib

[Zend Modules]
Xdebug

@treffynnon
Copy link
Collaborator

I have not tried to run Idiorm or Paris on PHP 5.5 so I don't know if that is the cause. Have you stepped through to find the source of the error?

@falmp
Copy link
Contributor

falmp commented Oct 1, 2013

Well, yes, the error is:

Fatal error: Call to a member function getAttribute() on a non-object in /tmp/idiorm/idiorm.php on line 326

Which is this piece of code:

protected static function _detect_identifier_quote_character($connection_name) {
    switch(self::$_db[$connection_name]->getAttribute(PDO::ATTR_DRIVER_NAME)) {
        case 'pgsql':
        case 'sqlsrv':
        case 'dblib':
        case 'mssql':
        case 'sybase':
        case 'firebird':
            return '"';
        case 'mysql':
        case 'sqlite':
        case 'sqlite2':
        default:
            return '`';
    }
}

I added a var_dump($connection_name, self::$_db);, which gave me:

string(7) "default"
array(1) {
  'default' =>
  NULL
}

I don't fully understand the Idiorm internals, not sure where it was supposed to initialize the self::$_db['default']. Any ideas?

@treffynnon
Copy link
Collaborator

Looks like the connection has either not been setup yet or it has been disconnected earlier than anticipated. I imagine that when you're running the tests that the order of tearDown and setUp methods in the tests are run in a different order to when I or PHPUnit run the tests. I would guess from that that there is probably a setUp in the tests that is not initializing the connection before attempting to access it.

@falmp
Copy link
Contributor

falmp commented Oct 2, 2013

I tried another approach with the lazy initialization which fixes my last problem with cached entities and also the test run, so I was able to run the tests under PHP 5.5.4:

$ phpunit 
PHPUnit 3.7.27 by Sebastian Bergmann.

Configuration read from /tmp/idiorm/phpunit.xml

...............................................................  63 / 200 ( 31%)
............................................................... 126 / 200 ( 63%)
............................................................... 189 / 200 ( 94%)
...........

Time: 953 ms, Memory: 5.75Mb

OK (200 tests, 226 assertions)

I proceeded with a pull request under #159 and I'd really appreciate it if you could pull this in.

@treffynnon
Copy link
Collaborator

I am going to go with @falmp pull request. @collegeman please could you test that his pull request (#159) also resolves your issue. I am pretty sure it will.

@treffynnon treffynnon closed this Nov 26, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants