Skip to content
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

Fix charset error when creating/editing routines; bug #13889 #13892

Merged
merged 1 commit into from Feb 20, 2018

Conversation

@williamdes
Copy link
Member

@williamdes williamdes commented Dec 26, 2017

Added an if in order to not add charset for VARBINARY and BINARY
Fixes the issue #13889

@williamdes williamdes closed this Dec 27, 2017
@williamdes williamdes reopened this Dec 27, 2017
@codecov
Copy link

@codecov codecov bot commented Dec 27, 2017

Codecov Report

Merging #13892 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master   #13892      +/-   ##
============================================
+ Coverage     55.44%   55.44%   +<.01%     
- Complexity    14359    14360       +1     
============================================
  Files           494      494              
  Lines         70455    70456       +1     
============================================
+ Hits          39061    39062       +1     
  Misses        31394    31394
@williamdes williamdes closed this Dec 27, 2017
@williamdes williamdes reopened this Dec 27, 2017
@ibennetch ibennetch changed the title Patch for bug #13889 Fix charset error when creating/editing routines; bug #13889 Dec 28, 2017
Copy link
Member

@nijel nijel left a comment

I think this is wrong approach - it should be rather fixed server side to not generate CHARSET part of the query in this case.

Your approach will lead to problems as well in case user changes the type several times - the charset field will be emptied leading to losing this information while editing.

@williamdes williamdes closed this Jan 19, 2018
@williamdes williamdes force-pushed the williamdes:patch-2 branch from 8fd0ff0 to 2823d27 Jan 19, 2018
@williamdes williamdes reopened this Jan 20, 2018
@williamdes williamdes closed this Jan 20, 2018
@williamdes williamdes reopened this Jan 20, 2018
@williamdes
Copy link
Member Author

@williamdes williamdes commented Feb 12, 2018

@ibennetch @nijel Can someone review my changes?

@nijel
Copy link
Member

@nijel nijel commented Feb 12, 2018

It looks better now, can you please adjust indentation of the nested block?

Fixes the issue #13889

Signed-off-by: William Desportes <williamdes@wdes.fr>
@williamdes williamdes force-pushed the williamdes:patch-2 branch from ce02b25 to 71e3047 Feb 12, 2018
@williamdes
Copy link
Member Author

@williamdes williamdes commented Feb 19, 2018

@nijel can you merge this PR ?

@nijel nijel merged commit 68ece7b into phpmyadmin:master Feb 20, 2018
5 checks passed
5 checks passed
DCO All commits have a DCO sign-off from the author
Scrutinizer Analysis: No new issues – Tests: passed
Details
codecov/patch 100% of diff hit (target 55.44%)
Details
codecov/project 55.44% (+<.01%) compared to 00c112b
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nijel
Copy link
Member

@nijel nijel commented Feb 20, 2018

Merged, thanks for your contribution!

@nijel nijel added this to the 4.8.0 milestone Feb 20, 2018
@nijel nijel self-assigned this Feb 20, 2018
nijel added a commit that referenced this pull request Feb 20, 2018
Signed-off-by: Michal Čihař <michal@cihar.com>
@williamdes williamdes deleted the williamdes:patch-2 branch Feb 23, 2018
@williamdes williamdes self-assigned this Feb 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.