Skip to content

[stable16] Correctly handle emtpy string in proxyuserpwd config - #17511

Merged
blizzz merged 3 commits into
stable16from
backport/16721/stable16
Oct 17, 2019
Merged

[stable16] Correctly handle emtpy string in proxyuserpwd config#17511
blizzz merged 3 commits into
stable16from
backport/16721/stable16

Conversation

@blizzz

Copy link
Copy Markdown
Member

backport of #16721

As documented, the default value for config value proxyuserpwd is ''.
However, that value results in the error:
"cURL error 5: Unsupported proxy syntax in '@'".
This patch handles the values of '' and null (the default in the code)
the same for config values proxyuserpwd and proxy.
Signed-off-by: Scott Shambarger <devel@shambarger.net>

@rullzerrullzer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Fine by me

@kesselbkesselb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please change the last return to return $proxyUserPwd . '@' . $proxyHost;. I cannot suggest a change for this line.

Comment threadlib/private/Http/Client/Client.php Outdated
if ($proxyUserPwd === '' || $proxyUserPwd === null) {
$proxyUri .= $proxyUserPwd . '@';
}
if ($proxyHost !== null) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Dead code

Comment threadlib/private/Http/Client/Client.php Outdated

if ($proxyUserPwd !== null) {
if ($proxyUserPwd === '' || $proxyUserPwd === null) {
$proxyUri .= $proxyUserPwd . '@';

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
$proxyUri .= $proxyUserPwd . '@';
return$proxyHost;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lets keep the backport as identical as we can to the original. Then we can do what you suggest in a PR against master ;)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

you suggest in a PR against master ;)

I already did ;) #14363

You cannot backport #16721 because it needs #14363 to be backported.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

#14363 is not totally needed, to fix the issue that proxy is set to '@' when the documented default values are used in config.php instead of null.

Comment threadlib/private/Http/Client/Client.php Outdated
}

$proxyUserPwd = $this->config->getSystemValue('proxyuserpwd', '');
$proxyUri = '';

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Dead code

@kesselb

Copy link
Copy Markdown
Contributor

Hmm. Tricky to backport because fd1d853 is not backported.

@blizzz

Copy link
Copy Markdown
MemberAuthor

@kesselb good catch. De-facto it the other commit is picked up half-way through merge conflict resolution, but some pieces are too much indeed. I ironed it out.

@blizzz
blizzz requested a review from kesselbOctober 11, 2019 13:55
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz
blizzzforce-pushed the backport/16721/stable16 branch from 6067e2e to 0f5cc52CompareOctober 11, 2019 14:09

@kesselbkesselb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. Tests need some work.

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz
blizzz merged commit 07a4b25 into stable16Oct 17, 2019
@blizzz
blizzz deleted the backport/16721/stable16 branch October 17, 2019 12:33
@rullzerrullzer mentioned this pull request Oct 28, 2019
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to reviewWaiting for reviewsbug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@blizzz@kesselb@rullzer@sshambar