use guzzle globally through a helper by individual-it · Pull Request #32505 · owncloud/core

individual-it · 2018-08-29T11:51:03Z

Description

use Guzzle through a Helper class
don't catch Guzzle exceptions all over the place, as there are not thrown any-more, but only check responses

Related Issue

owncloud/QA#586

Motivation and Context

have only one place where guzzle can be configured globally

How Has This Been Tested?

🤖

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Technical debt
Tests

Checklist:

Code changes
Unit tests added
Acceptance tests added
Documentation ticket raised:

Open tasks:

Backport (if applicable set "backport-request" label and remove when the backport was done)

patrickjahns · 2018-08-29T12:19:21Z

+	public static function sendRequest(
+		$url, $method='GET', $user=null, $password=null, $headers=null, $body = null, $config = null, $cookies = null
+	) {
+		$client = new Client();


Can we inject parameters from behat.yml into the helper?

Maybe, but what for?
The only thing that I can see that would make sense is the base part of the URL

Yes exactly - how do we define the server url currently?

Environment variable TEST_SERVER_URL and/or TEST_SERVER_FED_URL is set before running run.sh Then that is used to pass base_url in to Behat, and also BasicStructure constructor checks the 2 environment variables so it can set localBaseUrl and remoteBaseUrl to know where to find the local and remote (federated) server(s).

from wherever sendRequest() is called is responsible to create the complete URL

patrickjahns · 2018-08-29T14:12:59Z

Wondering if we should add a unit test to ensure the clientHelper works as designed?

codecov · 2018-08-30T05:47:58Z

Codecov Report

Merging #32505 into master will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master   #32505   +/-   ##
=========================================
  Coverage     64.12%   64.12%           
  Complexity    18643    18643           
=========================================
  Files          1176     1176           
  Lines         70189    70189           
  Branches       1270     1270           
=========================================
  Hits          45007    45007           
  Misses        24812    24812           
  Partials        370      370

Flag	Coverage Δ	Complexity Δ
#javascript	`52.89% <ø> (ø)`	`0 <ø> (ø)`	⬇️
#phpunit	`65.4% <ø> (ø)`	`18643 <ø> (ø)`	⬇️

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 001ddd6...77279b0. Read the comment docs.

individual-it · 2018-08-30T11:45:41Z

-		$responseHeaders = $response->getHeaders();
-		$tagUrl = $responseHeaders['Content-Location'][0];
-		$lastTagId = \substr($tagUrl, \strrpos($tagUrl, '/') + 1);
-		return ['lastTagId' => $lastTagId, 'HTTPResponse' => $response];


moved to Tags.php, so we always just return the response the whole way up till the place that cares

individual-it · 2018-08-30T11:49:14Z

-
-		$client = new Client();
-		$this->response = $client->get($url, $options);
+	private function checkDownload(


had to change that function to get the nice username & password

individual-it · 2018-08-30T11:51:03Z

@phil-davis @patrickjahns please review

phil-davis · 2018-08-30T15:34:22Z

+			'user' => $user,
+			'password' => $this->getPasswordForUser($user),
+		]);
+		$headers = ['Content-Type' => 'application/json'];


I guess the previous $client->post() with that json array of values had been automagically setting this header.

phil-davis

This looks like a great thing. It should help with the Guzzle6 PR, reducing the diff list a lot.

phil-davis · 2018-08-30T16:00:20Z

I pushed a commit with code style changes to make our "informal" phpcs pass.
And added some missing spaces either side of = signs.

I couldn't see any issues with the actual code refactoring - good work.

phil-davis · 2018-08-31T01:52:35Z

-		$body =
-			'<c:mkcalendar xmlns:c="urn:ietf:params:xml:ns:caldav" ' .
+		$body
+			= '<c:mkcalendar xmlns:c="urn:ietf:params:xml:ns:caldav" ' .


I think the previous code looks fine, but phpcs has a different idea.

individual-it · 2018-08-31T04:04:22Z

squashed and pushed again.
@patrickjahns for the unit tests I opened a new issue #32527

individual-it · 2018-08-31T04:09:29Z

backport in #32528

individual-it added 2 - Developing backport-request dev:acceptance-tests QA:team labels Aug 29, 2018

individual-it added this to the development milestone Aug 29, 2018

individual-it self-assigned this Aug 29, 2018

patrickjahns reviewed Aug 29, 2018

View reviewed changes

individual-it mentioned this pull request Aug 30, 2018

check correct upload reply code #32514

Merged

10 tasks

individual-it force-pushed the guzzleHelper branch 6 times, most recently from 4d7d01c to be88feb Compare August 30, 2018 11:24

individual-it commented Aug 30, 2018

View reviewed changes

individual-it changed the title ~~[WIP] use guzzle globally through a helper~~ use guzzle globally through a helper Aug 30, 2018

individual-it added 3 - To Review and removed 2 - Developing labels Aug 30, 2018

patrickjahns mentioned this pull request Aug 30, 2018

use official docker container for federaated servers #32413

Closed

9 tasks

phil-davis reviewed Aug 30, 2018

View reviewed changes

phil-davis approved these changes Aug 30, 2018

View reviewed changes

phil-davis reviewed Aug 31, 2018

View reviewed changes

use guzzle globally through a helper

77279b0

individual-it force-pushed the guzzleHelper branch from e74c4ab to 77279b0 Compare August 31, 2018 03:57

individual-it mentioned this pull request Aug 31, 2018

[stable10] use guzzle globally through a helper #32528

Merged

individual-it removed the backport-request label Aug 31, 2018

phil-davis merged commit e5d3b88 into master Aug 31, 2018

phil-davis deleted the guzzleHelper branch August 31, 2018 04:22

This was referenced Sep 4, 2018

acceptance tests - stop uploading when receiving an error #32559

Merged

don't expect exception from WebDavHelper::makeDavRequest owncloud/oauth2#160

Merged

lock Bot locked as resolved and limited conversation to collaborators Aug 31, 2019

Uh oh!

Conversation

individual-it commented Aug 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

Open tasks:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patrickjahns commented Aug 29, 2018

Uh oh!

codecov Bot commented Aug 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

individual-it Aug 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

individual-it commented Aug 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phil-davis left a comment

Choose a reason for hiding this comment

Uh oh!

phil-davis commented Aug 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

individual-it commented Aug 31, 2018

Uh oh!

individual-it commented Aug 31, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

individual-it commented Aug 29, 2018 •

edited

Loading

codecov Bot commented Aug 30, 2018 •

edited

Loading

individual-it Aug 30, 2018 •

edited

Loading