adding support for port parameter for the database

[levijackson]

mysqli passes the port in as a parameter versus mysql_connect()'s method of using the hostname http://localhost:8889. http://www.php.net/manual/en/mysqli.construct.php This change will allow you to specify a port in the config that gets used when setting up the connection. I found it useful when utilizing this with MAMP when the default port is 8889 for MySQL.

It can be set just like the other config settings by executing:
./bin/config set mysql.port 8889

Related libphutil change to support ports: phacility/libphutil#27

[epriestley]

Are the PhabricatorStorageManagementAPI methods ever called or used? It doesn't look like it at first glance. bin/storage probably needs to be updated to pass them to the $api, in the block where it sets host/user/pass/etc.

I generally think this is a good change, but I'd like to see more compatibility around it. For example, this setting has no effect when using the mysql (vs mysqli) extension, but there's no documentation about that, right? A better behavior for mysql might be:

• Check if there's a port in the host.
• If not, and the host doesn't start with "/" (indicating it's a socket path), append ":" and the port.

The mysql.port config option should be locked like the other configuration options are (setLocked(true)) so it's only editable from the CLI. This is a small security/policy thing, but the big motivator is so that a careless user can't break an install (by setting the port to something incorrect) without having access to be able to fix it.

For mysqli, it would be nice to accept either "host:port" (ignoring mysql.port) or "host" (respecting mysql.port) so that the engines are as compatible with one another as possible.

Does all of that sound reasonableish?

[levijackson]

Evan,
First off thanks for the detailed reply/feedback! That does sound reasonable to add that all in.

I wasn't completely sold on putting the parsing/appending of the port into libphutil, but I didn't see a better spot to do it. My concern with doing it where I have it now is that if the connection details were to be used elsewhere they would also need to parse it out. What are your thoughts on parsing it out and storing it in the config as if they had entered it in themselves?

https://github.com/levijackson/libphutil/commit/b748e84861b4c9938a3294ba624da31ee61b1530

Levi

[epriestley]

We can't generally edit config at runtime because it may be stored in files on disk that we can't reasonably modify -- is that what you're suggesting? Although "bin/config" could do some magic, I'd like to keep it as un-magical as we can.

We could raise a setup issue like:

Your `mysql.host` configuration includes a port, but this usage is deprecated. Specify the port in `mysql.port` instead.

Related Phabricator config:

    mysql.host  example.com:3306
    mysql.port  null

Run these commands:

    phabricator/ $ ./bin/config set mysql.host example.com
    phabricator/ $ ./bin/config set mysql.port 3306

(I can write that bit.)

We also probably need to default this to null instead of 3306, and let the mysql/mysqli code handle the implicit default -- this is more correct anyway, as it will respect PHP config like mysqli.default_port.

So I think we'd end up with this plan of attack:

• Default the config option to null, not to 3306.
• In the mysql connector in libphutil, append :port to the host if port is nonempty.
• In bin/storage, pass port to $api.
• Add setLocked(true) to the config option.
• After pulling, I'll write a setup issue to get ports out of mysql.host moving forward, without breaking anything which works today.

Oh, I missed that you updated, let me look at the modified change.

[epriestley]

Okay, I think the remaining items are (assuming I didn't misunderstand anything):

• In Phabricator, default to null instead of 3306. I'll pull after this change.
• In libphutil, adjust the port-respecting rules to be less forgiving: append ":port" in the mysql connector if port is nonempty and always be strict in mysqli. I'll pull after that.
• I'll write a setup issue to get people to always put port information in mysql.port.

This should leave everything that works today still working, and move us toward a brighter future where ports go in mysql.port all the time, I think?

[levijackson]

When you say to "always be strict in mysqli", does that mean to remove the code that would parse out the port if it was in the host string, and instead to just pass the host and port in without doing any checking of them? The setup issue message would then serve the purpose of educating users on the proper way of doing it?

[epriestley]

Yep, exactly.

[epriestley]

Oops, sorry, missed the update -- GitHub either doesn't send an email, or I have some configuration which disables it. Pulled as d27e7c5. I'll add the setup warning. Thanks!

[epriestley]

Here's the setup warning:

https://secure.phabricator.com/D6449