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

Psalm fixes 3 #17221

Merged
merged 7 commits into from Dec 3, 2021
Merged

Psalm fixes 3 #17221

merged 7 commits into from Dec 3, 2021

Conversation

kamil-tekiela
Copy link
Contributor

This is a collection of random fixes found through Psalm analysis

@codecov
Copy link

codecov bot commented Dec 1, 2021

Codecov Report

❗ No coverage uploaded for pull request base (master@ef72b89). Click here to learn what that means.
The diff coverage is 14.28%.

❗ Current head 9cae93d differs from pull request most recent head 053c98b. Consider uploading reports for the commit 053c98b to get more accurate results
Impacted file tree graph

@@            Coverage Diff            @@
##             master   #17221   +/-   ##
=========================================
  Coverage          ?   49.01%           
  Complexity        ?    16715           
=========================================
  Files             ?      579           
  Lines             ?    60499           
  Branches          ?        0           
=========================================
  Hits              ?    29656           
  Misses            ?    30843           
  Partials          ?        0           
Flag Coverage Δ
dbase-extension 48.79% <14.28%> (?)
recode-extension 48.80% <14.28%> (?)
unit-7.2-ubuntu-latest 48.71% <14.28%> (?)
unit-7.3-ubuntu-latest 51.43% <14.28%> (?)
unit-7.4-ubuntu-latest 51.53% <14.28%> (?)
unit-8.0-ubuntu-latest 51.57% <14.28%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...es/classes/Controllers/Export/ExportController.php 41.69% <0.00%> (ø)
libraries/classes/Database/Qbe.php 51.62% <0.00%> (ø)
libraries/classes/Database/Triggers.php 20.30% <0.00%> (ø)
libraries/classes/Display/Results.php 60.47% <ø> (ø)
libraries/classes/Server/Privileges.php 64.24% <ø> (ø)
libraries/classes/Utils/HttpRequest.php 78.64% <0.00%> (ø)
libraries/classes/Plugins/Export/ExportSql.php 80.31% <100.00%> (ø)

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 ef72b89...053c98b. Read the comment docs.

@kamil-tekiela kamil-tekiela force-pushed the Psalm-fixes-3 branch 2 times, most recently from a2674c8 to d0fc681 Compare December 2, 2021 21:52
libraries/classes/Database/Qbe.php Outdated Show resolved Hide resolved
libraries/classes/Database/Qbe.php Outdated Show resolved Hide resolved
@@ -102,7 +102,7 @@ private function handleContext(array $context)
];
if (strlen($this->proxyUser) > 0) {
$auth = base64_encode($this->proxyUser . ':' . $this->proxyPass);
$context['http']['header'] .= 'Proxy-Authorization: Basic '
$context['http']['header'] = 'Proxy-Authorization: Basic '
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I already reviewed this while checking code mistakes
I am not sure it should be changed
There was something that made this concat sill have some sense
Not sure what

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I will check it more carefully. I wasn't actually bothered to set up an environment for this to test as I need to disable CURL and enable allow_url_fopen. Let me see if it really throws an error as I'd expect it to.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked it and when using concatenation it throws Undefined array key "header" error. Maybe in the past the code looked different and the concatenation made sense. As of now, I can't see any possibility that the code does not throw a warning.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you

If I remember maybe this code is a bug because it would erase all the other headers
Not sure anymore

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that this looks like a bug. But if it is a bug then I do not know what was the desired behaviour. The name of the method is handleContext which doesn't tell us much about what the method should do. Let's remove the concatenation for now, and if someone wants to study this code in the future, they can fix the logic.

@MauricioFauth MauricioFauth added this to In progress in pull-requests via automation Dec 3, 2021
Signed-off-by: Kamil Tekiela <tekiela246@gmail.com>
Signed-off-by: Kamil Tekiela <tekiela246@gmail.com>
Signed-off-by: Kamil Tekiela <tekiela246@gmail.com>
Signed-off-by: Kamil Tekiela <tekiela246@gmail.com>
Signed-off-by: Kamil Tekiela <tekiela246@gmail.com>
The two globals in this method are read-only. There is an unused
parameter in the signature. Let's add one more and use them instead of globals.

Signed-off-by: Kamil Tekiela <tekiela246@gmail.com>
Signed-off-by: Kamil Tekiela <tekiela246@gmail.com>
pull-requests automation moved this from In progress to Reviewer approved Dec 3, 2021
@MauricioFauth MauricioFauth merged commit a428032 into phpmyadmin:master Dec 3, 2021
pull-requests automation moved this from Reviewer approved to Done Dec 3, 2021
@MauricioFauth
Copy link
Member

Merged, thanks for your contribution!

@MauricioFauth MauricioFauth self-assigned this Dec 3, 2021
@williamdes williamdes added this to the 5.2.0 milestone Dec 3, 2021
@kamil-tekiela kamil-tekiela deleted the Psalm-fixes-3 branch June 2, 2022 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
pull-requests
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants