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

Remove deprecated interface OCP\Files\Storage in favor of OCP\Files\S… #32051

Merged
merged 1 commit into from
Jul 23, 2018

Conversation

DeepDiver1975
Copy link
Member

…torage\IStorage

Description

interface OCP\Files\Storage is deprecated since owncloud 9 - let's remove it for owncloud 11

Motivation and Context

Cleanup deprecated APIs

How Has This Been Tested?

  • unit tests

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

@DeepDiver1975 DeepDiver1975 added this to the development milestone Jul 16, 2018
@DeepDiver1975 DeepDiver1975 self-assigned this Jul 16, 2018
@DeepDiver1975 DeepDiver1975 requested a review from PVince81 July 16, 2018 08:49
@DeepDiver1975 DeepDiver1975 force-pushed the tech-dept/deprecate-storage-interface branch from 2a017a9 to 641c663 Compare July 16, 2018 13:40
@codecov
Copy link

codecov bot commented Jul 16, 2018

Codecov Report

Merging #32051 into master will increase coverage by <.01%.
The diff coverage is 83.33%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #32051      +/-   ##
============================================
+ Coverage     63.85%   63.85%   +<.01%     
+ Complexity    18556    18555       -1     
============================================
  Files          1169     1169              
  Lines         69743    69741       -2     
  Branches       1267     1267              
============================================
  Hits          44535    44535              
+ Misses        24839    24837       -2     
  Partials        369      369
Flag Coverage Δ Complexity Δ
#javascript 52.81% <ø> (ø) 0 <ø> (ø) ⬇️
#phpunit 65.12% <83.33%> (ø) 18555 <99> (-1) ⬇️
Impacted Files Coverage Δ Complexity Δ
lib/private/Files/Storage/StorageFactory.php 100% <ø> (ø) 10 <0> (ø) ⬇️
lib/private/Files/Meta/MetaFileVersionNode.php 92.72% <ø> (ø) 27 <0> (ø) ⬇️
apps/files_sharing/lib/SharedStorage.php 83.72% <ø> (ø) 83 <0> (ø) ⬇️
lib/private/Files/External/PersonalMount.php 25% <ø> (ø) 4 <0> (ø) ⬇️
...b/private/Files/External/SessionStorageWrapper.php 0% <ø> (ø) 1 <0> (ø) ⬇️
lib/private/Files/FileInfo.php 91.91% <ø> (ø) 51 <0> (ø) ⬇️
apps/dav/lib/Connector/Sabre/File.php 85.9% <ø> (ø) 111 <0> (ø) ⬇️
lib/private/Files/Storage/FailedStorage.php 3.4% <0%> (ø) 44 <2> (ø) ⬇️
...iles/External/Auth/Password/SessionCredentials.php 0% <0%> (ø) 5 <1> (ø) ⬇️
lib/private/Files/External/ConfigAdapter.php 71.23% <0%> (ø) 20 <0> (ø) ⬇️
... and 13 more

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 1f8fb04...c2d096e. Read the comment docs.

@DeepDiver1975 DeepDiver1975 force-pushed the tech-dept/deprecate-storage-interface branch 2 times, most recently from f0ad120 to f7c2aed Compare July 17, 2018 08:08
Copy link
Contributor

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

Scrolled through the changes, looks good.

@DeepDiver1975 can you also grep through the known external storages apps (files_external_ftp, gdrive, dropbox and the primary object store ones) to make sure this interface wasn't used ?

did you have all known apps installed when checking ?

@DeepDiver1975 DeepDiver1975 force-pushed the tech-dept/deprecate-storage-interface branch from f7c2aed to c2d096e Compare July 19, 2018 18:55
@DeepDiver1975
Copy link
Member Author

files_external_ftp, gdrive, dropbox

based on flysystem -> handled in core

primary object store ones

based on IObjectStore

@DeepDiver1975 DeepDiver1975 merged commit 7fb3bbb into master Jul 23, 2018
@DeepDiver1975 DeepDiver1975 deleted the tech-dept/deprecate-storage-interface branch July 23, 2018 14:02
@phil-davis
Copy link
Contributor

phil-davis commented Jul 8, 2019

@DeepDiver1975 I found this PR when I was backporting "Introduce PHPstan" #35774
This PR removes the guts of

interface Storage extends IStorage {}

And that removed the declaration of the constructor.
Which meant that lib/private/Files/External/InvalidStorage.php no longer needed $parameters arg in its constructor.

The backport happily removed $parameters and in stable10 I got:

php -d zend.enable_gc=0 vendor-bin/phpstan/vendor/bin/phpstan analyse --memory-limit=2G --configuration=./phpstan.neon --level=0 lib/private/Files/External/InvalidStorage.php 
PHP Fatal error:  Declaration of OC\Files\External\InvalidStorage::__construct() must be compatible with OCP\Files\Storage::__construct($parameters) in /home/phil/git/owncloud/core/lib/private/Files/External/InvalidStorage.php on line 33

And I had to put the arg back (while making the backport).

What is the situation with this PR?
I guess it will never be released?
Or is there some way to remove deprecated code in the future?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants