Skip to content

Update guzzle to 6.3#29211

Closed
phil-davis wants to merge 9 commits into
masterfrom
update-guzzle-1
Closed

Update guzzle to 6.3#29211
phil-davis wants to merge 9 commits into
masterfrom
update-guzzle-1

Conversation

@phil-davis

@phil-davis phil-davis commented Oct 11, 2017

Copy link
Copy Markdown
Contributor

Description

Update Guzzle to 6.3, and also other libraries (deleted and regenerated composer.lock)

Related Issue

Motivation and Context

Guzzle 5.3 is outdated, needs update.
Also some libs used by apps will need Guzzle > 6.

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)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

This is a newer version of #28063 made by cherry-pick of the commits from there, resolving conflicts and regenerating composer.lock to (hopefully) be in sync with all the latest in composer.json then fixing remaining integration test fails and massaging it all into these 3 "logical" commits.

@phil-davis

phil-davis commented Oct 11, 2017

Copy link
Copy Markdown
Contributor Author

Note about getEffectiveUrl() in WebDav.php
https://github.com/owncloud/core/pull/29211/files#diff-dc84a6418f59a28b93942dd9dd005b6bL734
I removed the use of it (was just in a text message, I replaced it with other text). To get that sort of data from the response you now have to get hold of TransferStats. That was quite a few method calls down to do that, and then pass the answer all the way back... and it was easier just to not bother.

For how to:
https://stackoverflow.com/questions/30682307/how-to-read-the-response-effective-url-in-guzzle-6-0

$client = new \GuzzleHttp\Client();

$client->get('http://some.site.com', [
    'query'   => ['get' => 'params'],
    'on_stats' => function (TransferStats $stats) use (&$url) {
        $url = $stats->getEffectiveUri();
    }
])->getBody()->getContents();

echo $url; // http://some.site.com?get=params

http://docs.guzzlephp.org/en/latest/request-options.html#on-stats

and some light reading references that are handy to have:
http://docs.guzzlephp.org/en/stable/psr7.html
http://www.php-fig.org/psr/psr-7/

@phil-davis

Copy link
Copy Markdown
Contributor Author

PR #29205 has the original 3 commits from #28063 plus 2 more for the changes that I needed to make, so that can be seen if you like also.

@phil-davis phil-davis mentioned this pull request Oct 11, 2017
9 tasks
@phil-davis

Copy link
Copy Markdown
Contributor Author

@PVince81 @DeepDiver1975 @SergioBertolinSG this is the version to review.
And then decide how to proceed with it, because I guess there is some risk of breaking something???

  1. Do you want to backport it to have it out the door so that other apps quickly get Guzzle6 in their environment (and does that mean code changes in apps that need to be synced with the release of this)?
    or
  2. Plan release for 10.1

If (2) then we might be best to leave the PR here and not even merge to master just yet. Because it has a fair bit of change that could be annoying in master when we are doing other little things that need to be backported.

@PVince81

Copy link
Copy Markdown
Contributor

The way to move forward is as follows:

  • make sure all core stuff still works fine with this PR, this includes federated sharing
  • make sure all external storage apps' composer is updated to the same Guzzle v6 version and test again
  • announce in public that any apps using Guzzle will need updating for future OC version x.y.z
  • merge all the things and release version x.y.z

@phil-davis

Copy link
Copy Markdown
Contributor Author

Rebased and conflicts resolved. I suspect that there were some methods moved into TestHelpers so those might need Guzzle changes done...

@PVince81

PVince81 commented Nov 8, 2017

Copy link
Copy Markdown
Contributor

Should we plan to merge this for master, for 10.1 ? This means we'll also need to adjust external storage apps one by one to support 10.1. This also means we'll need two branches for every app: one compatible with OC 10.0.x and one with 10.1.x (likely master).

@phil-davis

Copy link
Copy Markdown
Contributor Author

stable10.0.* still seems to be in "active development", meaning that almost everything is back-ported there. Once the 10.0.* series seems to be really just getting bug-fixes, security fixes and only very minor behavior changes, and 10.1 is really coming in view - at that point I would merge.

Because until then, something like this will just be a nuisance to people who do code for master and then have to backport to stable10.

It should not be difficult to rebase it here every couple of weeks... (although I am finding a few recent refactorings that need to be touched in relation to this Guzzle stuff)

@codecov

codecov Bot commented Nov 8, 2017

Copy link
Copy Markdown

Codecov Report

Merging #29211 into master will decrease coverage by 0.02%.
The diff coverage is 70%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #29211      +/-   ##
============================================
- Coverage     62.61%   62.58%   -0.03%     
- Complexity    17584    17592       +8     
============================================
  Files          1038     1038              
  Lines         57920    57921       +1     
============================================
- Hits          36264    36249      -15     
- Misses        21656    21672      +16
Impacted Files Coverage Δ Complexity Δ
lib/private/Files/Storage/DAV.php 88.81% <ø> (ø) 163 <0> (ø) ⬇️
lib/private/Http/Client/Response.php 53.84% <100%> (ø) 6 <1> (ø) ⬇️
lib/private/Http/Client/Client.php 53.7% <66.66%> (-34.3%) 19 <0> (+2)
lib/private/Console/Application.php 0% <0%> (ø) 22% <0%> (+6%) ⬆️

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 ecf5f79...4aea94c. Read the comment docs.

@phil-davis phil-davis force-pushed the update-guzzle-1 branch 5 times, most recently from da55d29 to b6edf21 Compare November 10, 2017 02:54
@DeepDiver1975

Copy link
Copy Markdown
Member

conflict need to be resolved

@phil-davis

Copy link
Copy Markdown
Contributor Author

@PVince81 @DeepDiver1975
Conflicts resolved.

  1. As a result of having to merge changes in composer.json I ran composer update (on a dev system with PHP 5.6) and Sabre gets a bump from 4.1.2 to 4.1.3 - that caused test fails because there is hard-coded test data that has 4.1.2 embedded in it. For now I just did a search/replace to bump the version in the test data. For the future, do we want to somehow avoid having that specific version information embedded in test data?

  2. tests/integration/work/filespecificsize.txt is added in this PR (from its early days). Is there a reason for that? Or was it just an accident?

@phil-davis phil-davis force-pushed the update-guzzle-1 branch 5 times, most recently from 5d43a9a to ed9b87c Compare December 6, 2017 09:46
@phil-davis phil-davis mentioned this pull request Dec 6, 2017
9 tasks
@phil-davis

Copy link
Copy Markdown
Contributor Author

Code here has been massaged back into a set a 4 "logical" commits in PR #29773

@phil-davis phil-davis closed this Dec 7, 2017
@felixboehm felixboehm removed this from the triage milestone Apr 10, 2018
@PVince81 PVince81 deleted the update-guzzle-1 branch September 27, 2018 13:35
@lock lock Bot locked as resolved and limited conversation to collaborators Sep 27, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants