Fix PDO MySQL URI test #8451

mvorisek · 2022-04-28T08:14:18Z

"\0" char in URI causes the remaining part completely ignored, thus testing effectively only mysql:dbname=letshopeinvalid

but this URI then tested connection /w default host/port/credentials resulting with a many different failures (for example, consider MySQL server accessible on localhost /w and /wo default port)

ext/pdo_mysql/tests/pdo_mysql___construct_uri.phpt

cmb69 · 2022-04-28T10:41:49Z

Isn't that NUL character an important part of the test? Can you show the test failures?

mvorisek · 2022-04-28T10:51:32Z

I belive no, @cmb69 please test yourself

the important fact is this tests is not working correctly if the MySQL server is not on localhost/default port

if server port is for example 3307, I get:

SQLSTATE[HY000] [2002] No connection could be made because the target machine actively refused it

with this PR, I get [1049] Unknown database 'letshopeinvalid' correctly

cmb69 · 2022-04-28T12:26:48Z

if server port is for example 3307, I get:
SQLSTATE[HY000] [2002] No connection could be made because the target machine actively 

Why not add that to the list of expected failure messages?

mvorisek · 2022-04-28T12:29:22Z

because the test is wrong, we definitely do not want to touch other service than defined in DSN

cmb69 · 2022-04-28T12:37:06Z

"\0" char in URI causes the remaining part completely ignored

This appears to be the whole point of that part of the test. It is about "mysql:dbname=letshopeinvalid\0$dsn; that "letshopeinvalid" indicates that the rest of the DSN is supposed to be ignored. If we remove the NUL byte, we might as well drop that part of the test altogether.

mvorisek · 2022-04-28T12:42:38Z

"\0" char in URI causes the remaining part completely ignored

This appears to be the whole point of that part of the test. It is about "mysql:dbname=letshopeinvalid\0$dsn; that "letshopeinvalid" indicates that the rest of the DSN is supposed to be ignored. If we remove the NUL byte, we might as well drop that part of the test altogether.

maybe, would it be fine if I add something like chr(0) . ';charset=nonsense'?

test will be fixed and the "\0" behaviour will be kept tested (even if I am not 100% sure if that behaviour is per some spec)

cmb69 · 2022-04-28T12:54:28Z

maybe, would it be fine if I add something like chr(0) . ';charset=nonsense'?

If that would error without the NUL byte, that appears to be reasonable. Maybe @kamil-tekiela has thoughts on this.

even if I am not 100% sure if that behaviour is per some spec

"Spec" might be an exaggeration, but it tests the implementation (which is as it is for whatever reason):

php-src/ext/pdo/pdo.c

Lines 165 to 167 in 0df2886

    
           if (data_source[i] == '\0') { 
        
           	break; 
        
           }

kamil-tekiela · 2022-04-28T13:30:34Z

this test was completely wrong

Sorry, I am not seeing how it is wrong.

"\0" char in URI causes the remaining part completely ignored

That's the whole idea. The test is checking whether the URI past the NUL byte is ignored. If it's ignored then PDO should error out on the incorrect database name.

thus testing effectively only mysql:dbname=letshopeinvalid

It's testing whether the NUL byte ends DSN

but this URI then tested connection /w default host/port/credentials resulting with a many different failures (for example, consider MySQL server accessible on localhost /w and /wo default port)

Ok, that part might need fixing. The DSN should contain the same config except dbname which should be incorrect.

maybe, would it be fine if I add something like chr(0) . ';charset=nonsense'?

And what would that test? We hope that this test never reaches this part so invalid charset is meaningless to this test. Better keep that part as it was.

mvorisek · 2022-04-28T13:35:09Z

Sorry, I am not seeing how it is wrong.

you answered it by yourself:

but this URI then tested connection /w default host/port/credentials resulting with a many different failures (for example, consider MySQL server accessible on localhost /w and /wo default port)

Ok, that part might need fixing. The DSN should contain the same config except dbname which should be incorrect.

now the test is testing the behaviour as before & /w full DSN, can you please review the changes?

mvorisek · 2022-04-28T13:48:38Z

tested with #8392, I can confirm the changes are fixing the test

kamil-tekiela · 2022-04-28T21:24:30Z

ext/pdo_mysql/tests/pdo_mysql___construct_uri.phpt

@@ -73,5 +71,5 @@ MySQLPDOTest::skip();
    print "done!";
 ?>
 --EXPECTF--
-[003] URI=uri:file://%spdomuri.tst, DSN=mysql%sdbname=%s, File=%spdomuri.tst (%d bytes, 'mysql%sdbname=letshopeinvalid%s'), chr(0) test, EXPECTED ERROR
+[003] URI=uri:file://%spdomuri.tst, DSN=mysql:%sdbname=%s, File=%spdomuri.tst (%d bytes, 'mysql:%sdbname=letshopeinvalid%snonsense'), EXPECTED ERROR


Why did the output change?

, chr(0) test is not needed here as the line/echo is "identified" by [003], other additions here are to improve the expectation

mvorisek · 2022-05-02T07:33:57Z

can this PR be merged? I need it for #8392

cmb69 · 2022-05-02T08:21:22Z

ext/pdo_mysql/tests/pdo_mysql___construct_uri.phpt

-                fwrite($fp, sprintf('mysql:dbname=letshopeinvalid;%s%s',
-                    chr(0), $dsn));
+                $dsnUnknownDatabase = preg_replace('~dbname=[^;]+~', 'dbname=letshopeinvalid', $dsn)
+                    . chr(0) . ';host=nonsense;unix_socket=nonsense';
+                fwrite($fp, $dsnUnknownDatabase);


It seems to me that this change defeats the purpose of that part of the test. The test is for checking that a NUL byte terminates the DSN string. If there is no NUL byte (or it would be ignored), later options override previous options (dbname=good;dbname=bad would be the same as dbname=bad).

I fixed the test in a minimalistic way.

I aggre, we can basically copy the 1st test with fwrite($fp, $dsn. chr(0) . ';host=nonsense;unix_socket=nonsense');, not bother with dbname=letshopeinvalid and expect the same result. Do you want me to do that?

I aggre, we can basically copy the 1st test with fwrite($fp, $dsn. chr(0) . ';host=nonsense;unix_socket=nonsense');, not bother with dbname=letshopeinvalid and expect the same result.

That make sense to me.

done, 1st test vs chr(0) test:

CI issue unrelated, let's merge this PR, needed for #8392

mvorisek changed the title ~~Fix PDO URI test~~ Fix PDO Mysql URI test Apr 28, 2022

mvorisek commented Apr 28, 2022

View reviewed changes

ext/pdo_mysql/tests/pdo_mysql___construct_uri.phpt Show resolved Hide resolved

Fix PDO URI test

c6afdf8

mvorisek force-pushed the fix_mysql_uri_test branch from ecc9a01 to c6afdf8 Compare April 28, 2022 09:01

mvorisek marked this pull request as ready for review April 28, 2022 09:54

mvorisek changed the title ~~Fix PDO Mysql URI test~~ Fix PDO MySQL URI test Apr 28, 2022

keep testing chr(0) in DSN

d708bf7

mvorisek force-pushed the fix_mysql_uri_test branch from 6e7abf5 to d708bf7 Compare April 28, 2022 12:57

kamil-tekiela reviewed Apr 28, 2022

View reviewed changes

kamil-tekiela approved these changes Apr 29, 2022

View reviewed changes

mvorisek mentioned this pull request May 2, 2022

mysqlnd does not support utf8mb3 charset #8478

Open

cmb69 reviewed May 2, 2022

View reviewed changes

reuse 1st test to test chr(0) behaviour as much as possible

feed0db

mvorisek mentioned this pull request May 2, 2022

Test Windows x86 and x64 with Github Actions #8392

Closed

cmb69 closed this in 136ef6f May 3, 2022

mvorisek deleted the fix_mysql_uri_test branch May 3, 2022 18:14

Fix PDO MySQL URI test #8451

Fix PDO MySQL URI test #8451

Uh oh!

Conversation

mvorisek commented Apr 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

cmb69 commented Apr 28, 2022

Uh oh!

mvorisek commented Apr 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cmb69 commented Apr 28, 2022

Uh oh!

mvorisek commented Apr 28, 2022

Uh oh!

cmb69 commented Apr 28, 2022

Uh oh!

mvorisek commented Apr 28, 2022

Uh oh!

cmb69 commented Apr 28, 2022

Uh oh!

kamil-tekiela commented Apr 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mvorisek commented Apr 28, 2022

Uh oh!

mvorisek commented Apr 28, 2022

Uh oh!

kamil-tekiela Apr 28, 2022

Choose a reason for hiding this comment

Uh oh!

mvorisek Apr 29, 2022

Choose a reason for hiding this comment

Uh oh!

mvorisek commented May 2, 2022

Uh oh!

cmb69 May 2, 2022

Choose a reason for hiding this comment

Uh oh!

mvorisek May 2, 2022

Choose a reason for hiding this comment

Uh oh!

cmb69 May 2, 2022

Choose a reason for hiding this comment

Uh oh!

mvorisek May 2, 2022

Choose a reason for hiding this comment

Uh oh!

mvorisek May 3, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mvorisek commented Apr 28, 2022 •

edited

Loading

mvorisek commented Apr 28, 2022 •

edited

Loading

kamil-tekiela commented Apr 28, 2022 •

edited

Loading