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

Fixed urls in navigation #15941

Merged
merged 1 commit into from Mar 8, 2020
Merged

Conversation

@yashrajbothra
Copy link
Contributor

yashrajbothra commented Feb 9, 2020

Signed-off-by: Yash Bothra yashrajbothra786@gmail.com

Description

concatinated url string and removed from array

Fixes: #15893
Fixes: #15626

Copy link
Member

williamdes left a comment

I am not sure this fix it a good solution, can you try to fix the routing function or improve it?

@yashrajbothra

This comment has been minimized.

Copy link
Contributor Author

yashrajbothra commented Feb 9, 2020

Definately I will try to improve the function tomorrow.

I pushed this fix becuz all other urls follow similar concatenation ⬇️
image

@williamdes williamdes added this to In progress in pull-requests via automation Feb 9, 2020
@williamdes williamdes added this to the 5.1.0 milestone Feb 9, 2020
@yashrajbothra

This comment has been minimized.

Copy link
Contributor Author

yashrajbothra commented Feb 10, 2020

Hey I tried multiple possible solution but all had some consequenses so only this one fits the best.

Can you suggest anything @williamdes or should we call other members to review

@williamdes

This comment has been minimized.

Copy link
Member

williamdes commented Feb 11, 2020

@yashrajbothra did you see

return 'index.php?route=' . $route . self::getCommon($additionalParameters, '&');

calls

return htmlspecialchars(
self::getCommonRaw($params, $divider)
);

maybe add a parameter so it calls

public static function getCommonRaw($params = [], $divider = '?')
directly

@yashrajbothra

This comment has been minimized.

Copy link
Contributor Author

yashrajbothra commented Feb 11, 2020

Yes I was debugging and saw all of those but the problem is when we call getCommonRaw it uses

$query = http_build_query($params, '', $separator);

so the return value cannot have any $ and this results to error when we use vsprint to generate link ahead

@williamdes

This comment has been minimized.

Copy link
Member

williamdes commented Feb 11, 2020

Yes I was debugging and saw all of those but the problem is when we call getCommonRaw it uses

$query = http_build_query($params, '', $separator);

so the return value cannot have any $ and this results to error when we use vsprint to generate link ahead

htmlspecialchars seems to be what blocks the code

@yashrajbothra

This comment has been minimized.

Copy link
Contributor Author

yashrajbothra commented Feb 11, 2020

vsprintf contains false because link doesnt contain instructed format.
image

And here we can see that format is changed by this function
image

As a result we cant see any link 😕

@williamdes

This comment has been minimized.

Copy link
Member

williamdes commented Feb 11, 2020

vsprintf contains false because link doesnt contain instructed format.
image

And here we can see that format is changed by this function
image

As a result we cant see any link

htmlspecialchars seems to be what blocks the code

Did you remove the function htmlspecialchars to try ?
Did you try to change the https://www.php.net/manual/en/function.http-build-query.php enc_type param ?

@yashrajbothra

This comment has been minimized.

Copy link
Contributor Author

yashrajbothra commented Feb 12, 2020

Did you remove the function htmlspecialchars to try ?

@williamdes Yes, I did try but it didnt worked.

Did you try to change the https://www.php.net/manual/en/function.http-build-query.php enc_type param ?

Yes, I tried both encoding types but i dindn't help 🤔

Signed-off-by: Yash Bothra <yashrajbothra786@gmail.com>
@williamdes williamdes force-pushed the yashrajbothra:issue-15893 branch from edd61be to 40665b1 Mar 7, 2020
@codecov

This comment has been minimized.

Copy link

codecov bot commented Mar 7, 2020

Codecov Report

Merging #15941 into master will increase coverage by 0.64%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master   #15941      +/-   ##
============================================
+ Coverage     51.55%    52.2%   +0.64%     
+ Complexity    14958    14879      -79     
============================================
  Files           471      468       -3     
  Lines         62177    61285     -892     
============================================
- Hits          32058    31991      -67     
+ Misses        30119    29294     -825
pull-requests automation moved this from In progress to Reviewer approved Mar 7, 2020
Copy link
Member

williamdes left a comment

Best solution for now, thank you

@williamdes williamdes self-assigned this Mar 7, 2020
@williamdes williamdes requested a review from mauriciofauth Mar 8, 2020
Copy link
Member

mauriciofauth left a comment

I'm fine with this change if it fixes the issue. However I think we should find another solution that doesn't require to build the query string manually.

@williamdes williamdes merged commit abe0ae7 into phpmyadmin:master Mar 8, 2020
1 of 3 checks passed
1 of 3 checks passed
continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
codeclimate 2 issues to fix
Details
DCO DCO
Details
pull-requests automation moved this from Reviewer approved to Done Mar 8, 2020
@williamdes

This comment has been minimized.

Copy link
Member

williamdes commented Mar 8, 2020

Thank you so much for the fix @yashrajbothra !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
pull-requests
  
Done
Linked issues

Successfully merging this pull request may close these issues.

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