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

Phalcon 4 issue with TINYINT(1) #14708

Closed
daniel18387 opened this issue Jan 14, 2020 · 10 comments
Closed

Phalcon 4 issue with TINYINT(1) #14708

daniel18387 opened this issue Jan 14, 2020 · 10 comments
Assignees

Comments

@daniel18387
Copy link

@daniel18387 daniel18387 commented Jan 14, 2020

I have noticed an issue when saving records to tables in MySQL. Of the tables which have field columns with the data type TINYINT(1), I can see the values in the rows default to 1 when a row gets saved, regardless of the value given.

This only occurs when the mask is set to 1.

@ruudboon

This comment has been minimized.

Copy link
Member

@ruudboon ruudboon commented Jan 14, 2020

Can you give us a dump of your database structure and some basic code to reproduce this?

@ruudboon ruudboon added this to Current Sprint (Ends January 24th) in Phalcon Roadmap Jan 14, 2020
@daniel18387

This comment has been minimized.

Copy link
Author

@daniel18387 daniel18387 commented Jan 14, 2020

DB table mp_kyc_documents:

+--------------+---------------------+------+-----+---------+-------+
| Field        | Type                | Null | Key | Default | Extra |
+--------------+---------------------+------+-----+---------+-------+
| id           | bigint(20) unsigned | NO   | PRI | NULL    |       |
| user_id      | bigint(20) unsigned | NO   | MUL | NULL    |       |
| type         | tinyint(1) unsigned | YES  |     | 1       |       |
| status       | tinyint(1) unsigned | YES  |     | 1       |       |
| file_id      | bigint(20) unsigned | YES  |     | NULL    |       |
| created_on   | datetime            | YES  |     | NULL    |       |
| processed_on | datetime            | YES  |     | NULL    |       |
+--------------+---------------------+------+-----+---------+-------+

Code used:

$detail = [
    'id' => 123456,
    'userId' => 1234567,
    'type' => 2,
    'status' => 3,
];

$kycDocument = (new Models\MP\KycDocuments)
    ->setId($detail['id'])
    ->setUserId($detail['userId'])
    ->setType($detail['type'])
    ->setStatus($detail['status']);

$kycDocument->save();

After the save, both the type and status fields will set to 1. This has occurred since our systems upgraded to version 4 yesterday.

@ruudboon

This comment has been minimized.

Copy link
Member

@ruudboon ruudboon commented Jan 14, 2020

Thnx for the info.
We will investigate this. Looks like tinyint(1) is handled as boolean.
If you have time could try to save with value 0 for type of status?

https://github.com/phalcon/cphalcon/blob/master/phalcon/Db/Adapter/Pdo/Mysql.zep#L141

Looks like we made the assumption tinyint(1) is a boolean. Something I assumed as wel since MySql will save a boolean column as tinyint(1). Just looked up the specs but tinyint is handled as a normal tinyint (-128 through 127) but will only display 1 digit.

@daniel18387

This comment has been minimized.

Copy link
Author

@daniel18387 daniel18387 commented Jan 14, 2020

You are correct. It's being treated as a boolean value but it shouldn't be. As you've pointed out, it can go all the way up to 127 when signed and 255 when unsigned. It's worth noting that the (1) is only a display width. It doesn't control the width of the integer. https://dev.mysql.com/doc/refman/8.0/en/numeric-type-attributes.html

Key point from the the MySQL docs: The display width does not constrain the range of values that can be stored in the column. Nor does it prevent values wider than the column display width from being displayed correctly. For example, a column specified as SMALLINT(3) has the usual SMALLINT range of -32768 to 32767, and values outside the range permitted by three digits are displayed in full using more than three digits.

Therefore, this is a bug.

This has been brought up before on Phalcon too. See #2012

@daniel18387

This comment has been minimized.

Copy link
Author

@daniel18387 daniel18387 commented Jan 14, 2020

If anyone else is experiencing this issue, remove the mask from the integer data type to solve it your end before a fix is in place. i.e. Remove (1) from TINYINT(1) to leave a data type of TINYINT.

@ruudboon ruudboon self-assigned this Jan 14, 2020
@ruudboon ruudboon mentioned this issue Jan 14, 2020
4 of 5 tasks complete
@niden

This comment has been minimized.

Copy link
Member

@niden niden commented Jan 14, 2020

Resolved in #14709

Thank you @daniel18387

@niden niden closed this Jan 14, 2020
Phalcon Roadmap automation moved this from Current Sprint (Ends January 24th) to Implemented Jan 14, 2020
@niden niden moved this from Implemented to Released in Phalcon Roadmap Jan 26, 2020
@Ruzgfpegk

This comment has been minimized.

Copy link

@Ruzgfpegk Ruzgfpegk commented Jan 27, 2020

On the other hand, those who used TINYINT(1) as a BOOLEAN (because declaring a column as BOOL in MySQL Workbench outputs a TINYINT(1)) may have welcomed the v4.0.0 behavior.

Wouldn't a setting to specify the wished behavior be a better option in the long run?

@daniel18387

This comment has been minimized.

Copy link
Author

@daniel18387 daniel18387 commented Jan 27, 2020

On the other hand, those who used TINYINT(1) as a BOOLEAN (because declaring a column as BOOL in MySQL Workbench outputs a TINYINT(1)) may have welcomed the v4.0.0 behavior.

Wouldn't a setting to specify the wished behavior be a better option in the long run?

That doesn't make sense. Phalcon should work with MySQL, not necessarily MySQL Workbench (Third party product). i.e. Follow MySQLs documentation. MySQL Workbench (Which I use) should adhere to the standards, as should Phalcon. Any other suggestion is pure conjecture.

@Ruzgfpegk

This comment has been minimized.

Copy link

@Ruzgfpegk Ruzgfpegk commented Jan 27, 2020

MySQL Workbench is made by Oracle, so it's a first-party product (relative to MySQL Server).

The documentation for the numeric types says indeed:

The display width does not constrain the range of values that can be stored in the column.

But it also says just above:

This optional display width may be used by applications to display integer values having a width less than the width specified for the column by left-padding them with spaces. (That is, this width is present in the metadata returned with result sets. Whether it is used is up to the application.)

If you agree that "let the application do what it wants with it" is a standard, as it's what's written in the MySQL docs, then it's precisely what I'm suggesting in order to avoid said conjectures.

In the same documentation they mention the mapping they're doing from BOOLEAN to TINYINT, which is what their other own product Workbench does (to TINYINT(1)) and how the DB works: if you INSERT a TRUE (without '') into a TINYINT column it's understood (...and stored) as a 1.
This is an unfortunate choice on their end, because if you INSERT a TRUE you expect to SELECT a TRUE as well and there's no CAST(column AS BOOLEAN) available either, but this what we have to deal with, and we have a chance to "adhere to the[ir] standards" by letting that choice "up to the application".

If I can make my position more clear, the same way we have a:
\Phalcon\Mvc\Model::setup(['castOnHydrate' => true]);
which is not enabled by default due to certain issues, we could have something like:
\Phalcon\Mvc\Model::setup(['tinyintAsBool' => true]);
for those using TINYINT this way and nobody would lose any functionality.

@daniel18387

This comment has been minimized.

Copy link
Author

@daniel18387 daniel18387 commented Jan 28, 2020

Thank you for clarifying.

While the above sounds like a good idea to cover all bases, it's also redundant with this now in place. I use both boolean and integer values with TINYINT, which as mentioned above MySQL advocates. However, it begs the question why Phalcon should be more restrictive when it already allows the functionality for both.

In my personal opinion, I would prefer Phalcon's product to be more flexible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Phalcon Roadmap
  
Released
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.