-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
adding support for port parameter for the database #356
Conversation
Are the PhabricatorStorageManagementAPI methods ever called or used? It doesn't look like it at first glance. 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
The For Does all of that sound reasonableish? |
… out port in hostname for mysqli and to insert port into hostname for mysql
Evan, 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 |
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:
(I can write that bit.) We also probably need to default this to So I think we'd end up with this plan of attack:
Oh, I missed that you updated, let me look at the modified change. |
Okay, I think the remaining items are (assuming I didn't misunderstand anything):
This should leave everything that works today still working, and move us toward a brighter future where ports go in |
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? |
Yep, exactly. |
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! |
Here's the setup warning: |
See: phacility#356 Reviewed by: epriestley
Summary: A pull from GitHub recently added `mysql.port`, for explicitly configuring the MySQL port. See: - phacility/libphutil#27 - phacility#356 Add a setup warning for old-style configurations (which will still work properly), to get them to move to the new style. Test Plan: {F50113} Reviewers: btrahan, chad Reviewed By: chad CC: aran Differential Revision: https://secure.phabricator.com/D6449
See: phacility/phabricator#356 Reviewed by: epriestley
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