Remove connect_type setting #12950

Merged
merged 3 commits into from Feb 6, 2017

Projects

None yet

3 participants

@nijel
Member
nijel commented Feb 2, 2017

It is really not necessary as MySQL decides connection type rather based
on hostname than on anything else.

Signed-off-by: Michal Čihař michal@cihar.com

Before submitting pull request, please check that every commit:

  • Has proper Signed-Off-By
  • Has commit message which describes it
  • Is needed on it's own, if you have just minor fixes to previous commits, you can squash them
  • Any new functionality is covered by tests
@nijel nijel Remove connect_type setting
It is really not necessary as MySQL decides connection type rather based
on hostname than on anything else.

Signed-off-by: Michal Čihař <michal@cihar.com>
1b7914a
@ibennetch
Contributor

If, for some reason, we don't merge this, we should address an interesting situation where a user must use connect_type='socket' in order for $cfg['Servers'][$i]['socket'] to take effect. It makes sense why it was implemented this way, but if this doesn't get merged we can improve that.

@ibennetch
Contributor

Anyway, I like this and think it's a good improvement. I don't see a downside of merging this and it works for me.

@ibennetch

I have a few suggestions for documentation language.

doc/config.rst
@@ -219,7 +219,9 @@ Server connection settings
.. note::
The hostname ``localhost`` is handled specially by MySQL and it
- ignores :config:option:`$cfg['Servers'][$i]['port']` in this case.
+ ignores :config:option:`$cfg['Servers'][$i]['port']` in this case
@ibennetch
ibennetch Feb 3, 2017 edited Contributor

I'd make this chunk "The hostname localhost is handled specially by MySQL and it uses the socket based connection protocol. To use TCP/IP networking, use an IP address or hostname such as 127.0.0.1 or db.example.com. You can configure socket address by :config:option:$cfg['Servers'][$i]['socket']."

doc/config.rst
@@ -418,6 +425,13 @@ Server connection settings
:type: string
:default: ``'tcp'``
+ .. deprecated:: 4.7.0
+
+ This settings is no longer used as of 4.7.0, the MySQL anyway decides
@ibennetch
ibennetch Feb 3, 2017 Contributor

I suggest "This setting is no longer used as of 4.7.0, since MySQL decides the"

doc/config.rst
+
+ This settings is no longer used as of 4.7.0, the MySQL anyway decides
+ connection type based on host, so it could lead to unexpected results.
+ Please set :config:option:`$cfg['Servers'][$i]['host']` accordingly
@ibennetch
ibennetch Feb 3, 2017 Contributor

It's helpful how you direct users to 'host' here instead, good job.

@codecov-io
codecov-io commented Feb 3, 2017 edited

Codecov Report

Merging #12950 into QA_4_7 will increase coverage by 0.01%.

@@            Coverage Diff             @@
##           QA_4_7   #12950      +/-   ##
==========================================
+ Coverage   54.23%   54.24%   +0.01%     
==========================================
  Files         466      466              
  Lines       69577    69550      -27     
==========================================
- Hits        37733    37727       -6     
+ Misses      31844    31823      -21

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 736e206...162d135. Read the comment docs.

@ibennetch @nijel ibennetch Improve documentation on socket or tcp/ip setup
Signed-off-by: Michal Čihař <michal@cihar.com>
34946a2
@nijel
Member
nijel commented Feb 4, 2017

Applied your suggestions in 34946a2.

@nijel nijel self-assigned this Feb 4, 2017
@ibennetch
Contributor

Sorry I missed this, I think this line would be better as "can configure the path to the socket with"

Member
nijel replied Feb 6, 2017

Adjusted in 162d135

@nijel nijel Clarify socket connection docs
Signed-off-by: Michal Čihař <michal@cihar.com>
162d135
@ibennetch
Contributor

Great, I think this looks ready.

Do you really want to apply it to QA_4_7? I don't object because the modifications are relatively minor, but in general I think we should avoid such changes to a QA branch.

@nijel
Member
nijel commented Feb 6, 2017

I'm not sure, but I thought it is still reasonable to do that for 4.7 now.

@ibennetch
Contributor

Sure, let's go ahead with putting it in 4.7.

@nijel
Member
nijel commented Feb 6, 2017

Okay, merging.

@nijel nijel merged commit 9b534e5 into phpmyadmin:QA_4_7 Feb 6, 2017

2 of 3 checks passed

codecov/patch 28.57% of diff hit (target 54.23%)
Details
codecov/project 54.24% (+0.01%) compared to 736e206
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nijel nijel deleted the nijel:connect_type branch Feb 6, 2017
@nijel nijel added this to the 4.7.0 milestone Feb 6, 2017
@ibennetch
Contributor

Great. Did you use --author to add my name to this commit or is that part of the new Github review feature? Either way, it's pretty helpful!

@nijel
Member
nijel commented Feb 6, 2017

I've used commandline with -author

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