Skip to content

Fix #15093: Add additional field type metadata for pdo_mysql in getColumnMeta response. - #15114

Open
deAtog wants to merge 9 commits into
php:masterfrom
deAtog:GH15093
Open

deAtog wants to merge 9 commits into
php:masterfrom
deAtog:GH15093

Conversation

@deAtog

@deAtog deAtog commented Jul 26, 2024

Copy link
Copy Markdown

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.

Comment thread NEWS Outdated
@deAtog
deAtog force-pushed the GH15093 branch 2 times, most recently from ecb1f1a to 19b7a9e Compare July 26, 2024 17:42
@devnexen

Copy link
Copy Markdown
Member

You need to update/fix the following test

========DIFF========
--
         [flags] => Array
             (
                 [0] => not_null
016+             [1] => no_default_value
017+             [2] => num
             )
     
020+     [native_flags] => 36865
         [table] => test_33689
         [name] => bar
         [len] => 11
--
========DONE========
FAIL PDO MySQL Bug #33689 (query() execute() and fetch() return false on valid select queries) [ext/pdo_mysql/tests/bug_33689.phpt] 

maybe also adding a new test to demonstrate the full extent of your proposal, what do you think ?

@SakiTakamachi

Copy link
Copy Markdown
Member

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']);

@devnexen devnexen Jul 29, 2024 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: that or that might be ways around it. e.g. sorting. even for the type display, you re using EXPECTF after all.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fair enough, anyhow the maintainer will review all properly soon :)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

/* Should we set NUM_FLAG (libmysql does it) ? */
if (
(meta->type <= MYSQL_TYPE_INT24 &&
(meta->type != MYSQL_TYPE_TIMESTAMP || meta->length == 14 || meta->length == 8)
) || meta->type == MYSQL_TYPE_YEAR)
{
meta->flags |= NUM_FLAG;
}

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();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add support for all flags from mysql in the PDO MySql driver in the getColumnMeta function.

3 participants