PHPUnit database connection is not properly decoded

Looking at this during Drupalmountaincamp.

I can reproduce it on a D9.4; a couple of things to note:

• RFC 3986 mentions that
  

  3.2.1. User Information
  ...
  Use of the format "user:password" in the userinfo field is
  deprecated.

• PHP apparently supported usernames with @ and passwords with slashes at some point via parse_url; however this is not the case any more (read more here). The following code would return false:
  

  $url = "ftp://usernameisanemail@emailservername.com:passwordhassymbols$%?@ftp.testserver03.com/";
  var_dump(parse_url($url));
  

  That is in accord with what the PHP manual mentions regarding return values:

  On seriously malformed URLs, parse_url() may return false.

• To easily reproduce this issue, one could simply change the password for the MySQL user to something that contains a slash and then modify the SIMPLETEST_DB uri inside phpunit.xml. That would effectively cause the reported error when running any test that uses a db uri string connection. I was able to reproduce it like this:

  .././vendor/bin/phpunit --filter testSpecifyDbUrl core/modules/system/tests/src/Kernel/Scripts/DbCommandBaseTest.php
  

My suggestion would be to do two things:

1. Enhance the createConnectionOptionsFromUrl() to report something more specific when parse_url is not happy
2. Enhance the testGetInvalidArgumentExceptionInUrlConversion() testing function to cover the unhappy case

('Seriously malformed connection URL provided');
Not sure about this saying. If an exception is thrown is that not already "serious"
This error doesn't really tell me what the problem was. How is a user suppose to know it was '/'?

Tagging for usability for their thoughts also.

There's a #ux channel we can post if this doesn't get traction eventually.

Thanks!

Good point.
However, we are not gonna get more information out of parse_url; there is more info about that and references in #6
The wording itself comes from PHP's manual and ultimately I don't see any options for wording and/or UX enhancements here.

To me this feels like an issue of technical implementation.

Looking at the example strings I feel that relaying on parse_url and failing what is not compatible with a chosen function is something to be fixed by changing error messages.

It needs annoyingly more work but if I look at the example URLs they would be splitable with something like explode using ":" the one part with "@" and the one with port and db with "/". Some cases you can go around this kind of problems by base64 encoding stuff before passing it as URL and the decoding after splitting. These kind of workarounds probably sound silly and are prone to create other issues but the point is the example cases are manageable. We shouldn't report a valid URL as malformed to user if we just check it against a single function response and not against the definition.

The least thing would be to return "Seriously malformed or unsupported connection URL provided". For debugging purposes of course it would be usable to be told what was submitted and how it was understood (split by the function) and the solution would be more or less obvious.

(This reminds me a bit of all the failed domain transfers where the automation didn't accept autogenerated EEP keys that had "'" in them.)

We talked about this in usability meeting today, but didn't have time to come into a conclusion about final recommendation.

I did however notice another thing:

Does this issue actually affect also /core/modules/sqlite/src/Driver/Database/sqlite/Connection.php where $url is parsed with parse_url() even if the username and password are after that unset?

PGSQL doesn't seem to override the createConnectionOptionsFromUrl function.

Sorry for posting a lot of comments while digging deeper. I noticed that Database.php before it calls createConnectionOptionsFromURL in concertDbURLConnectionInfo already does some validation for the URL. For instance a preg_match to check if schema exists in $url and if not, throwing

throw new \InvalidArgumentException("Missing scheme in URL '$url'");

Thus I think the validation for password and other possible issues should happen already there prior to calling createConnectionOptionsFromURL. Then I think we would also cover the SQLite version without duplicating code.

In concertDbURLConnectionInfo() the error messages seem include the $url variable which is in my opinion helpful for the administrator/sitebuilder to fix the issue.

Moving to NW as it doesn't seem the remaining task has been achieved.

@smustgrave:

When you tag an issue for usability review, please make it easy for the usability team to review the issue. I am removing the tag for now. If you still want usability review after updating the issue summary, then you can add the tag again.

In #9, you mentioned the string Seriously malformed connection URL provided. If that is shown to the user, even when running texts, then it needs attention. The issue summary does not include any steps to reproduce, so I am not sure if that is why you asked for usability review.

I can give general advice for error messages: they should be descriptive and actionable. In this case, the "descriptive" part should be an explanation that some characters, although valid for MySQL, are not supported by Drupal. The "actionable" part should suggest using a different password.

I am adding the tag for an issue summary update. Since this issue is a bug report, it should include steps to reproduce. The Proposed resolution should be more specific. For example, if we are adding a comment to settings.php, then what is the proposed text? If one of the Remaining tasks is to finalize the text of an exception message, then include a draft version of the text in the Proposed resolution.

Strange this is not fixed even in Drupal 11, and there not a lot of people complaining.

I can confirm this is still present, my password contains '#'. If I leave it like that, url is invalid, and if encode password it is not properly decoded.

You can encode password with urlencode (php function) or just find online url encoder - encode password only.

Attaching patch fix, that will decode url properly. There are a lot of comments telling about this issue on php.net parse_url page.

Also I think I covered #15

For instance if your password is mysql://user:pass#pass@host/db it should become `mysql://user:pass%23pass@host/db` to become valid url

IS still needs to be updated

Hopefully this is good enough.
I've rewritten everything with new approach.
Hopefully no one will be mad, this issue were not going anywhere anyway.

Sorry should of also meant that patches should be in MRs

Looking at #17 will need a test case showing the issue.