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

The integer id field is being populated as a string after save #13002

Closed
ChangePlaces opened this issue Aug 2, 2017 · 12 comments

Comments

Projects
7 participants
@ChangePlaces
Copy link

commented Aug 2, 2017

Expected and Actual Behavior

When I save a model, the id field is automatically populated with the id value, however, instead of it being an integer value, it's populated as a string value. When outputting json this is an issue. In the database, the id field is an integer auto_increment primary key, and in the annotations, the `@column' type is an integer. (I'm using the annotation strategy)

e.g. instead of seeing: "id":5, I get "id":"5"

Details

  • Phalcon version: (php --ri phalcon) 3.2.1
  • PHP Version: (php -v) 7
  • Operating System:
  • Installation type: Compiling from source
  • Zephir version (if any):
  • Server: Nginx
  • Other related info (Database, table schema):
@virgofx

This comment has been minimized.

Copy link
Member

commented Aug 2, 2017

Yes, good catch. We need to respect column map types and cast appropriately right here as MySQL lastInsertId() always returns string

https://github.com/phalcon/cphalcon/blob/master/phalcon/mvc/model.zep#L2432-L2434

Always returns string
http://php.net/manual/en/pdo.lastinsertid.php

@ChangePlaces

This comment has been minimized.

Copy link
Author

commented Aug 2, 2017

Ahh I thought lastinsertid may have been the root cause but never had time to check! I'm still new to Phalcon so wouldn't know how to check the mapped type! Thanks for the work on narrowing it down.

@magroski

This comment has been minimized.

Copy link

commented Feb 19, 2018

Any updates on this?

@stale stale bot added the stale label May 20, 2018

@stale stale bot closed this May 21, 2018

@zeecher

This comment has been minimized.

Copy link

commented Oct 3, 2018

Anything on this ?

@niden niden reopened this Oct 4, 2018

@niden

This comment has been minimized.

Copy link
Member

commented Oct 4, 2018

@zeecher I will check on this this coming week after I attend some other pressing matters.

Thank you for reporting this

@stale stale bot removed the stale label Oct 4, 2018

@dreanmer

This comment has been minimized.

Copy link

commented Oct 4, 2018

this is really annoying

@Jeckerson

This comment has been minimized.

Copy link
Member

commented Oct 4, 2018

What is complete version of MySQL? And show output of

SHOW VARIABLES LIKE 'sql_mode';

@zeecher

This comment has been minimized.

Copy link

commented Nov 26, 2018

for now solved it by overriding model's toArray

public function toArray($columns = null): array
   {
       $array = parent::toArray($columns);

       $metadata = $this->getDI()->get('modelsMetadata');

       $types = $metadata->getDataTypes($this);

       if((isset($types['id']) && isset($array['id'])) && $types['id'] === Column::TYPE_INTEGER) {

           $array['id'] = (int)$array['id'];
       }

       return $array;
   }
@virgofx

This comment has been minimized.

Copy link
Member

commented Nov 26, 2018

The proper solution is to just cast to (int) here:

https://github.com/phalcon/cphalcon/blob/master/phalcon/mvc/model.zep#L2486-L2489

PHP returns string from MySQL's internal call which returns bigint. There's no reason why we shouldn't just cast to int in the code ...

@magroski

This comment has been minimized.

Copy link

commented Nov 26, 2018

This gets really annoying when using castOnHydrate, because it will cast when you do a find, but won't when saving. So we end up having to manually cast to int whenever we use an id field.

@virgofx

This comment has been minimized.

Copy link
Member

commented Dec 19, 2018

I've been talking about this with the core team.

The issues at play are:

  • MySQL PDO options -- Are we emulating prepares? If so, (default) , everything is string. If false , PDO returns native PHP types. This is similar to when Phalcon uses cast_on_hydrate for SELECTs.
  • Phalcon global property cast_on_hydrate = true ... Which forces SELECTs to cast appropriately via cloneResultMap(). Note: This option should not be used in combination with EMULATE_PREPARES = false as it would decrease performance having both MySQL driver + Phalcon doing the same thing.

If either if the above are true. I believe ... that we should properly handle for the following PDO extensions:

MySQL:
"With no argument, LAST_INSERT_ID() returns a BIGINT UNSIGNED (64-bit) value representing the first automatically generated value successfully inserted for an AUTO_INCREMENT column as a result of the most recently executed INSERT statement."

MariaDB:
"LAST_INSERT_ID() (no arguments) returns the first automatically generated value successfully inserted for an AUTO_INCREMENT column as a result of the most recently executed INSERT statement."

Thus, with respect to MySQL/MariaDB ... lastInsertId() should always be sufficient to fit inside a PHP 64bit integer. However, we can't do this for 100% of the cases (e.g. users that don't fit into the top two bullets listed above).

So we could introduce a new global value orm.cast_last_insert_id_to_int that would solve it for all the unique cases. Otherwise, Phalcon would have to do much work to determine the type of PDO connection (as well as potential emulation options) as other PDOs could have different sequence values.

I guess the other thought is we could pull the column map for the field and if it's INT and cast_on_hydrate we could convert there. However, for those that have cast_on_hydrate set to false for improved performance and are using native MySQL emulation (thus, the value SHOULD be an int) .... then this wouldn't be the case and extra logic would be required to determine emulation mode + (MySQL or MariaDB).

Thus, the reason for the explicit and separate global which removes any crazy logic in terms of attempting to autodetect how to handle this case.

Thus, potential solution would be a 3 line change with effectively zero performance impact. For example:

            // NEW CODE HERE --- Opt in and would fix
            if (globals_get("orm.cast_last_insert_id_to_int") {
                lastInsertId = intval(lastInsertId, 10);
            }

Thoughts?

Ping @phalcon/core-team

@phalcon phalcon deleted a comment from stale bot Dec 23, 2018

@niden niden self-assigned this Dec 23, 2018

@niden niden added the Enhancement label Dec 23, 2018

@niden niden added this to To do in 4.0 Release via automation Dec 23, 2018

@stale stale bot added the stale label Mar 23, 2019

@phalcon phalcon deleted a comment from stale bot Mar 23, 2019

@stale stale bot removed the stale label Mar 23, 2019

@niden niden moved this from To do to In progress in 4.0 Release May 10, 2019

@niden niden referenced this issue May 13, 2019

Merged

T13002 last insert #14068

3 of 4 tasks complete

niden added a commit that referenced this issue May 15, 2019

niden added a commit that referenced this issue May 15, 2019

niden added a commit that referenced this issue May 15, 2019

niden added a commit that referenced this issue May 15, 2019

niden added a commit that referenced this issue May 15, 2019

niden added a commit that referenced this issue May 15, 2019

niden added a commit that referenced this issue May 15, 2019

niden added a commit that referenced this issue May 15, 2019

niden added a commit that referenced this issue May 15, 2019

niden added a commit that referenced this issue May 15, 2019

niden added a commit that referenced this issue May 15, 2019

niden added a commit that referenced this issue May 15, 2019

niden added a commit that referenced this issue May 15, 2019

niden added a commit that referenced this issue May 15, 2019

niden added a commit that referenced this issue May 15, 2019

niden added a commit that referenced this issue May 15, 2019

niden added a commit that referenced this issue May 15, 2019

niden added a commit that referenced this issue May 15, 2019

@niden

This comment has been minimized.

Copy link
Member

commented May 15, 2019

Resolved in #14068

@niden niden closed this May 15, 2019

4.0 Release automation moved this from In progress to Done May 15, 2019

SidRoberts added a commit to SidRoberts/cphalcon that referenced this issue May 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.