kohana / orm

Kohana ORM
159 stars 108 forks source link

PHP 5.6.21 find_all #129

Closed farazmunj closed 8 years ago

farazmunj commented 8 years ago

Hi after upgrading to PHP 5.6.21 find_all will return objects but _original_values is empty and new values are loaded as _changed so ->pk() function return NULL

$model = new Model_Status ();
                $results = $model->find_all ();
                foreach ( $results as $result ) {
                    self::$_table [$result->pk ()] = ( object ) $result->as_array ();
                }

object Model_Status(58) {
   protected _changed => array(4) (
        "id" => string(2) "id"
        "name" => string(4) "name"
        "code" => string(4) "code"
        "message" => string(7) "message"
    )
protected _primary_key => string(2) "id"
protected _primary_key_value => NULL
Luoti commented 8 years ago

I'm also having some problems with the ORM and PHP 5.6.21.

My problem is that when a model has relation to some other model, loading the model gives:

Kohana_Exception [ 0 ]: The othermodel:id property does not exist in the Model_Somemodel class

My guess is that these problems are somewhat related, because they both appeared after the PHP update. The problem is not the same, but I wrote this to also let you know you are not alone.

Let's see if we can fix it.

neo22s commented 8 years ago

Happens the same on 7.0.6 https://github.com/kohana/core/issues/687

I am getting crazy to debug this. Only happens when you do a find_all().

I am checking the http://www.php.net/ChangeLog-7.php#7.0.6 changelog but I can't see where...

Luoti commented 8 years ago

If I use the old database driver "MySQL" instead of "MySQLi", my problem goes away. Can you test yours?

Of course there is the deprecation error, but I went over that with error_reporting(E_ALL & ~E_DEPRECATED); in the index.php.

farazmunj commented 8 years ago

I solve this issue by downgrading to 5.6.20, so at least I have a working site, It looks like in this update mysqli_result::fetch_object create object first and then assign values to it so they behave as updated values not values loaded from DB and as Original values. I run a test on 5.6.20 and 5.6.21 and different is clear. In 5.6.21 change log there is no change for mysqli driver but there is a update under Postgres that relate to it. https://bugs.php.net/bug.php?id=71820

neo22s commented 8 years ago

You are right I was stuck here yesterday https://github.com/kohana/database/blob/3.3/master/classes/Kohana/Database/MySQLi/Result.php#L46

Biggest issue I am facing is debugging it :O

Basically I had done the same and went down to php 7.0.5 now setting up a new testing environment :(

lets hope I get some time today.

Anyone else can take a look?

oliverservin commented 8 years ago

It seems commit https://github.com/php/php-src/commit/1b632cfe834bfd87d182566d7d960df7d10ded72 passed this time on PHP 5.6.21 and 7.0.6 but fixes were rejected before, https://bugs.php.net/bug.php?id=48487 and https://bugs.php.net/bug.php?id=61843.

There is a related open bug regarding this, mysqli_fetch_object changed behaviour from 5.6.20.

neo22s commented 8 years ago

ok,..... so how the F*\ we fix this? :O

farazmunj commented 8 years ago

I will not say use backtrack. to detect if ORM is called using mysqli_result::fetch_object but we can check if submitted values e.g. id is primary key for table set it as primery_key_val and mark object as loaded. and when saving object instead of using insert or update query use insert with on duplicate key update, Its a big update on ORM behavior but if this bug/change is not fixed/reversed its our only option to use ORM

Luoti commented 8 years ago

I think for me the easiest solution will be switching to PDO-driver.

It went pretty smoothly by switching the driver and adding 'identifier' => '`', to the database config file.

Then I copied the list_columns(), datatype() and list_tables() functions from Kohana_Database_MySQLi to application/classes/Database/PDO.php.

Not the most elegant solution, but it seems to work.

I still wonder what the PHP-team will do about the issue. Now that the PDO and MySQLi drivers behave differently. I wonder which one will they "fix"?

neo22s commented 8 years ago

Good point @Luoti we may use the PDO but then will be changed also in next release?

Can we please all vote this issue so they take a look? https://bugs.php.net/bug.php?id=72151

neo22s commented 8 years ago

Funny now I realize that in the PHP docs they mention:

Note that mysqli_fetch_object() sets the properties of the object before calling the object constructor.

http://php.net/manual/en/mysqli-result.fetch-object.php

Luoti commented 8 years ago

Are we (the Kohana ORM users) only ones affected? I did some Googling and was ready to find all kinds of bug reports about PHP applications breaking, but for now, the only people suffering about this bug seems to be us. Have you found other projects that also relay on the mysqli_fetch_object() to work the way it is documented?

neo22s commented 8 years ago

not at all! no one else complains...

I found a nasty way to make it work....I have tried many things but at the end I like this the most.

In the results.php in function current()

Instead of returning the object directly like does now: return $this->_result->fetch_object($this->_as_object, (array) $this->_object_params);

We can do this nasty thing:

// Return an object of given class name
            $ob = $this->_result->fetch_object($this->_as_object, (array) $this->_object_params);

            //mark as loaded if ORM
            if (isset(class_parents($ob)['ORM']) AND method_exists($ob,'verify_loaded'))
                $ob->verify_loaded();

            return $ob;

and in orm.php add the function

    /**
     * nasty bug to check if model is loaded after mysqli_fetch_object PHP 5.6.21 and PHP 7.0.6
     * used in common/Database_MySQLi_Result.php function current
     * see https://github.com/open-classifieds/openclassifieds2/issues/1864
     * @return void 
     */
    public function verify_loaded()
    {
        // Flag as loaded, valid and get primary key value
        if ($this->_loaded == FALSE AND isset($this->_object[$this->_primary_key]))
        {
            $this->_loaded = $this->_valid = TRUE;
            $this->_primary_key_value = $this->_object[$this->_primary_key];
        }    
    }

just testing it.... seemed the easiest no backtrace, not using anything like PHP versions, works in other PHP versions too. is quite clean and so far makes a bit of sense.

Do you like it? as a temporary solution....

neo22s commented 8 years ago

@Luoti what I am not sure if we should set _valid=TRUE too... maybe not doing it breaks other stuff? damn...

oliverservin commented 8 years ago

It looks fine and seems to work well, maybe as a temporary solution since it seems someone is already taking a look at that PHP bug report.

[2016-05-09 13:42 UTC] ondrej@php.net -Assigned To: +Assigned To: ab [2016-05-09 13:42 UTC] ondrej@php.net This affects both 5.6.21 and 7.0.6 as well.

https://bugs.php.net/bug.php?id=72151

neo22s commented 8 years ago

@oliverds yes I saw it, but next release of PHP maybe is in 1 month....and this fix will work for those whom update even later I.

Not sure what todo with the _valid I've been checking code and should not affect anything else...but I am afraid we use it somewhere like with the loaded :(

cdp1337 commented 8 years ago

This fix seems to work for me as well, thanks for publishing it!

Out of curiosity, why does everything in Kohana-land seem to omit '{' and '}' braces? In numerous places I've found code such as

foreach($a as $b)
    if($b.something)
        throw
            new Exception('msg');

Is this syntax from the original Kohana coding style or what?

entermix commented 8 years ago

@cdp1337 This style of writing code to Kohana: http://kohanaframework.org/3.3/guide/kohana/conventions

neo22s commented 8 years ago

Hello,

Sorry to tell this but the fix I provided does not work in all scenarios.

Now we found that in the same case if you want to ->save() the $this->_changed is empty therefore instead of doing an update seems to try to do an insert....

Driving me crazy :(

neo22s commented 8 years ago

ok, found the issue....

New code to verify loaded

    /**
     * nasty bug to check if model is loaded after mysqli_fetch_object PHP 5.6.21 and PHP 7.0.6
     * used in common/Database_MySQLi_Result.php function current
     * see https://github.com/open-classifieds/openclassifieds2/issues/1864
     * @return void 
     */
    public function verify_loaded()
    {
        // Flag as loaded, valid and get primary key value
        if ($this->_loaded == FALSE AND isset($this->_object[$this->_primary_key]))
        {
            $this->_loaded = $this->_valid = TRUE;
            $this->_primary_key_value = $this->_object[$this->_primary_key];
        }    
    }

_primary_key_value was missing...took me like 2 hours debugging line by line :(

Luoti commented 8 years ago

Thanks for doing that!

It would be great if we wouldn't have to rely on the mysqli_fetch_object function to work the way it does. Apparently most of the projects do not count on it, and there is obvious pressure to change the mysqli_fetch_object function, because it has been proposed for 3 times and now even changed once.

sigma-technology commented 8 years ago

Just encountered this issue when I blindly ran a yum update - it's quite annoying that PHP made such a substantial change in a maintenance version! Just wanted to say thanks to @neo22s for your hard work on coming up with a fix.

For anyone running a Unix server (CentOS7 here), I chose to just downgrade PHP back to 5.6.20...

yum downgrade php\*

Don't forget to restart Apache: systemctl restart httpd

Hope this helps someone!

neo22s commented 8 years ago

Closing since PHP already released a new version.

PHP 7.0.7 Released - PHP: News Archive - 2016 - http://j.mp/22qgzE9

https://bugs.php.net/bug.php?id=72151

@sigmatec welcome!

sigma-technology commented 8 years ago

Just wondering as I'm now paranoid about upgrading, but has anyone tested 7.0.7 and 5.6.22 to make sure this is resolved (without using neo's fix).

farazmunj commented 8 years ago

I tested it with 5.6.22 and its working without neo's fix. we can start his patch for some other day.

sigma-technology commented 8 years ago

Thanks @farazilu - I can also confirm that it's working fine for me.