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

Correctly handle emtpy string in proxyuserpwd config #16644

Closed
wants to merge 0 commits into from
Closed

Correctly handle emtpy string in proxyuserpwd config #16644

wants to merge 0 commits into from

Conversation

sshambar
Copy link

@sshambar sshambar commented Aug 2, 2019

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

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.

This is cleaner IMO. since it sets the correct default

lib/private/Http/Client/Client.php Outdated Show resolved Hide resolved
lib/private/Http/Client/Client.php Outdated Show resolved Hide resolved
lib/private/Http/Client/Client.php Outdated Show resolved Hide resolved
@rullzer rullzer added 2. developing Work in progress bug labels Aug 5, 2019
@rullzer rullzer added this to the Nextcloud 17 milestone Aug 5, 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.

Awesome :)

@rullzer rullzer added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Aug 6, 2019
@kesselb
Copy link
Contributor

kesselb commented Aug 6, 2019

$this->config
->expects($this->at(0))
->method('getSystemValue')
->with('proxy', null)
->willReturn('foo');
$this->config
->expects($this->at(1))
->method('getSystemValue')
->with('proxyuserpwd', null)
->willReturn(null);

@rullzer do you know why these tests are not failing? getSystemValue is invoked with a different default value 🤔

@rullzer
Copy link
Member

rullzer commented Aug 6, 2019

$this->config
->expects($this->at(0))
->method('getSystemValue')
->with('proxy', null)
->willReturn('foo');
$this->config
->expects($this->at(1))
->method('getSystemValue')
->with('proxyuserpwd', null)
->willReturn(null);

@rullzer do you know why these tests are not failing? getSystemValue is invoked with a different default value thinking

I think because null is not matched exactly in the with statement...

Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

empty might also have done the trick but this is fine :)

Thanks a lot!

@ChristophWurst
Copy link
Member

I think because null is not matched exactly in the with statement...

one could then try

$this->config 
 			->expects($this->at(1)) 
 			->method('getSystemValue') 
 			->with($this->equalTo('proxyuserpwd'), $this->equalTo(null)) 
 			->willReturn(null); 

That might help in case phpunit filters out empty args.

@MorrisJobke MorrisJobke mentioned this pull request Aug 8, 2019
28 tasks
@rullzer rullzer closed this Aug 11, 2019
@rullzer
Copy link
Member

rullzer commented Aug 11, 2019

Ah my bad. pushed wrong. let me create a proper branch.

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.

4 participants