Skip to content

Conversation

@blizzz
Copy link
Member

@blizzz blizzz commented Oct 11, 2019

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>
@blizzz blizzz added bug 3. to review Waiting for reviews labels Oct 11, 2019
@blizzz blizzz added this to the Nextcloud 16.0.6 milestone Oct 11, 2019
Copy link
Member

@rullzer rullzer left a comment

Choose a reason for hiding this comment

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

Fine by me

Copy link
Collaborator

@kesselb kesselb left a comment

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.

if ($proxyUserPwd === '' || $proxyUserPwd === null) {
$proxyUri .= $proxyUserPwd . '@';
}
if ($proxyHost !== null) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Dead code


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

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
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
Collaborator

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
Member Author

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.

}

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

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
Collaborator

kesselb commented Oct 11, 2019

Hmm. Tricky to backport because fd1d853 is not backported.

@blizzz
Copy link
Member Author

blizzz commented Oct 11, 2019

@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 kesselb October 11, 2019 13:55
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz blizzz force-pushed the backport/16721/stable16 branch from 6067e2e to 0f5cc52 Compare October 11, 2019 14:09
Copy link
Collaborator

@kesselb kesselb left a comment

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 stable16 Oct 17, 2019
@blizzz blizzz deleted the backport/16721/stable16 branch October 17, 2019 12:33
@rullzer rullzer mentioned this pull request Oct 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review Waiting for reviews bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants