Repository navigation
Conversation
ecb1f1a to
19b7a9e
Compare
|
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.
…method for the pdo_mysql driver.
…a() method for the pdo_mysql driver.
…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.