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
Closed

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

ChangePlaces opened this issue Aug 2, 2017 · 12 comments
Assignees
Labels
enhancement Enhancement to the framework

Comments

@ChangePlaces
Copy link

ChangePlaces 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
Copy link
Contributor

virgofx 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
Copy link
Author

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
Copy link

Any updates on this?

@stale stale bot added the stale Stale issue - automatically closed label May 20, 2018
@stale stale bot closed this as completed May 21, 2018
@zeecher
Copy link

zeecher commented Oct 3, 2018

Anything on this ?

@niden niden reopened this Oct 4, 2018
@niden
Copy link
Member

niden 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 Stale issue - automatically closed label Oct 4, 2018
@dreanmer
Copy link

dreanmer commented Oct 4, 2018

this is really annoying

@Jeckerson
Copy link
Member

Jeckerson commented Oct 4, 2018

What is complete version of MySQL? And show output of

SHOW VARIABLES LIKE 'sql_mode';

@zeecher
Copy link

zeecher 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
Copy link
Contributor

virgofx 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
Copy link

magroski 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
Copy link
Contributor

virgofx 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 Enhancement to the framework label Dec 23, 2018
@stale stale bot added the stale Stale issue - automatically closed label Mar 23, 2019
@phalcon phalcon deleted a comment from stale bot Mar 23, 2019
@stale stale bot removed the stale Stale issue - automatically closed label Mar 23, 2019
@niden niden mentioned this issue May 13, 2019
4 tasks
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
Copy link
Member

niden commented May 15, 2019

Resolved in #14068

@niden niden closed this as completed May 15, 2019
@niden niden added the 4.0 label Jun 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement to the framework
Projects
None yet
Development

No branches or pull requests

7 participants