Fix #15093: Add additional field type metadata for pdo_mysql in getColumnMeta response. #15114
Conversation
…method for the pdo_mysql driver.
…a() method for the pdo_mysql driver.
|
You need to update/fix the following test maybe also adding a new test to demonstrate the full extent of your proposal, what do you think ? |
|
Sorry for the late reply, I was not feeling well. I'll check this later. |
|
|
||
| // libmysql and mysqlnd will show the pdo_type entry at a different position in the hash | ||
| // and will report a different type, as mysqlnd returns native types. | ||
| unset($m['pdo_type']); |
There was a problem hiding this comment.
note: that or that might be ways around it. e.g. sorting. even for the type display, you re using EXPECTF after all.
There was a problem hiding this comment.
This section was pulled from another test. As stated in the comment, the value of the pdo_type may differ based on which driver is used. I only modify the flags and added a native_flags attribute, which is really what the test is meant to cover.
There was a problem hiding this comment.
fair enough, anyhow the maintainer will review all properly soon :)
There was a problem hiding this comment.
Is it possible to split the tests per driver and check the value of pdo_type in the test to ensure I am not breaking existing functionality?
These behaviors don't change from driver to driver, just the values change, so I think it's okay to just check it with mysqlnd testing.
| #define IS_PRI_KEY(n) ((n) & PRI_KEY_FLAG) | ||
| #define IS_NOT_NULL(n) ((n) & NOT_NULL_FLAG) | ||
| #define IS_BLOB(n) ((n) & BLOB_FLAG) | ||
| #define IS_NUM(t) ((t) <= FIELD_TYPE_INT24 || (t) == FIELD_TYPE_YEAR || (t) == FIELD_TYPE_NEWDECIMAL) |
There was a problem hiding this comment.
IS_NUM condition has changed. What impact does the removed condition have?
In other words, what happens in the new logic if the value is FIELD_TYPE_YEAR or FIELD_TYPE_NEWDECIMAL?
There was a problem hiding this comment.
IS_NUM was declared, but never used by mysqlnd. I suppose this would be a breaking change though for anything that imports this header file externally and uses this definition. Internally, mysqlnd sets the NUM_FLAG under the following conditions:
php-src/ext/mysqlnd/mysqlnd_wireprotocol.c
Lines 1221 to 1228 in 0e33b8d
However, MySql sets the NUM_FLAG based on their definition of IS_NUM here:
https://github.com/mysql/mysql-server/blob/596f0d238489a9cf9f43ce1ff905984f58d227b6/include/mysql.h#L116-L118
Maybe the condition in mysqlnd should be changed to use the condition provided by mysql?
| --SKIPIF-- | ||
| <?php | ||
| require_once __DIR__ . '/inc/mysql_pdo_test.inc'; | ||
| MySQLPDOTest::skip(); |
There was a problem hiding this comment.
MySQLPDOTest::skipNotMySQLnd();
if (MYSQLPDOTest::isPDOMySQLnd()) die("skip libmysql only test");
By using the above codes, you can separate the tests for mysqlnd and libmysql.
IMHO, these features are less common and less likely to be bug-reported, so it is desirable to be able to cover all cases with tests.
There was a problem hiding this comment.
I changed the code to use skipNotMySQLnd(), but in retrospect I think you were suggesting to write two separate tests. Given the differences regarding the NUM_FLAG above, separating them does seem like a good idea at first. However, one would expect the two to be functionally equivalent in this test.
…st the mysqlnd driver." This reverts commit ba7c8d2 as it causes the test to fail.
This fixes #15093 by checking for and adding additional flags to the output from getColumnMeta. It also adds a 'native_flags' attribute that returns the unmodified flags from mysql should the flags be expanded in the future.