Gdrive lib update#37265
Conversation
|
Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes. |
|
Note: I adjusted PR #37186 the other day to get the latest Also PR #37011 "Guzzle6 for core" - |
d13a46f to
9434776
Compare
Codecov Report
@@ Coverage Diff @@
## master #37265 +/- ##
============================================
- Coverage 64.56% 64.56% -0.01%
- Complexity 19218 19220 +2
============================================
Files 1268 1268
Lines 75104 75091 -13
Branches 1331 1331
============================================
- Hits 48493 48484 -9
+ Misses 26219 26215 -4
Partials 392 392
Continue to review full report at Codecov.
|
|
|
|
There is a known issue when a file is re-uploaded (just upload the same file twice or more): the file is duplicated in the gdrive with the same name, which will be problematic (we don't handle multiple files with the same name) It seems the problem is with the ownCloud's .part file. Disabling the .part files in gdrive in https://github.com/owncloud/core/blob/master/apps/dav/lib/Connector/Sabre/File.php#L637 would solve the problem because the file will be uploaded directly to the previous file, overwriting it. |
|
I am adding @micbar so he knows about this PR and may add his thoughts |
|
@jvillafanez what is the question? |
|
The question is if we want to spend time refactoring https://github.com/owncloud/core/blob/master/apps/dav/lib/Connector/Sabre/File.php#L637 in order to provide a good solution, or if we include directly the storage name there to prevent using .part files in google drive. |
2deb9ff to
3b1ce44
Compare
|
IMHO .part files should be always in the temp folder and then upload to the external storage once is finished (and even checked by the antivirus if enabled) |
|
Decided to add the google storage to the core's ".part file exclusion list" for now in order to progress with the PR. |
|
This is also related to #36509 (comment) for PHP 7.4 support. |
|
I added this to the 10.5.0 project. It needs review and moving forward. |
* Use json encode/decode to transfer tokens to the UI * Use new API changes * Modify change detection for folders Fix wrong function call
e0be20b to
b14e8e4
Compare
|
I rebased to get this up-to-date with other stuff that has happened in core master in the last week. @jvillafanez @micbar what happens next? This needs to move forward - it is becoming a blocker if we need it for 10.5.0 |
|
Code-wise this is ready. It still need review. |
|
@jvillafanez I requested a review. Hope that helps ;-) |
|
|
||
| private function getDefaultFieldsForFile() { | ||
| return 'id,name,mimeType,parents,capabilities/canEdit,size,viewedByMeTime,createdTime,modifiedTime'; | ||
| static $fileFields = null; // keep static variable to avoid recreating the string |
There was a problem hiding this comment.
IMO static is kinda functional thing. class constant suits better here
There was a problem hiding this comment.
I'm not sure what it the best option. The problem with the class constant is that is easy that the field list becomes unreadable if a string is used. If we use an array, we'll have to build the string every time.
Another alternative could be to use a class attribute, but for me it would be polluting the class with information that isn't part of the class. In addition, the attribute could be changed in any part of the class.
I think this is a way the keep balance between readability and performance. The string is built only once when needed, and using an array makes it easier to read. In addition, all this complexity is isolated inside the method, without polluting the class.
|
|
||
| private function getDefaultFieldsForFolderScan() { | ||
| return 'incompleteSearch,nextPageToken,files(id,name,mimeType,parents,capabilities/canEdit,size,viewedByMeTime,createdTime,modifiedTime)'; | ||
| static $folderFields = null; // keep static variable to avoid recreating the string |
There was a problem hiding this comment.
the same as above. could be a class property.
karakayasemi
left a comment
karakayasemi
left a comment
There was a problem hiding this comment.
Reviewing deeply is not easy. The changeset looks okay, basic file operations and authorization are working.
| return $this->info->getSize(); | ||
| $size = $this->info->getSize(); | ||
| if ($size < 0) { | ||
| return null; |
There was a problem hiding this comment.
this invalidates the current PHPDoc for getSize
|
|
7 participants
Description
First commit is the library update, both google and icewind.
The second commit contains the actual changes to adapt to the API changes.
The third commit is just readability improvements.
Note that change detection has been changed because the API no longer provides a id per change, so using the google's change API was a problem. It probably isn't suited for the use case implemented in the code.
Related Issue
#36509 (comment)
Motivation and Context
Update google's library version to be compatible with latest php 7.4 deprecations.
How Has This Been Tested?
Tested manually basic file operations through ownCloud.
Screenshots (if appropriate):
Types of changes
Checklist:
Requires testing for gdrive and SMB connectors due to library updates