Skip to content

Conversation

@icewind1991
Copy link
Member

…pers are applied to the source storage

the target storage doesn't need additional handling for wrappers as the wrappers implementation of moveFromStorage already deals with that

Any storage based on local storage isn't affected by this as local storage already has it's own way of handling with this

You can test this by using groupfolders on a nextcloud instance using object storage, currently this will use the fallback "copy+delete" path in that case instead of a simple rename

…pers are applied to the source storage

the target storage doesn't need additional handling for wrappers as the wrappers implementation of moveFromStorage already deals with that

Any storage based on local storage isn't affected by this as local storage already has it's own way of handling with this

Signed-off-by: Robin Appelman <robin@icewind.nl>
@icewind1991 icewind1991 added the 3. to review Waiting for reviews label Sep 25, 2019
@icewind1991 icewind1991 added this to the Nextcloud 18 milestone Sep 25, 2019
@icewind1991 icewind1991 requested a review from rullzer September 25, 2019 17:18
* @var Jail $sourceStorage
*/
$sourceInternalPath = $sourceStorage->getUnjailedPath($sourceInternalPath);
$sourceStorage = $sourceStorage->getUnjailedStorage();
Copy link
Member

Choose a reason for hiding this comment

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

why do this? It is not used

Copy link
Member Author

Choose a reason for hiding this comment

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

it is, in the next loop iteration

Copy link
Member

Choose a reason for hiding this comment

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

right it loops... 🤦‍♂️

Copy link
Collaborator

@kesselb kesselb Sep 25, 2019

Choose a reason for hiding this comment

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

I thought the same .... Pretty nice the while we loop the storage wrappers up thing.

private function isSameStorage(IStorage $storage): bool {
while ($storage->instanceOfStorage(Wrapper::class)) {
/**
* @var Wrapper $sourceStorage
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* @var Wrapper $sourceStorage
* @var Wrapper

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.

Looks good. And testing on objectstore with groupfolders didn't do 💥

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.

LGTM

@rullzer rullzer merged commit cc6874d into master Sep 26, 2019
@rullzer rullzer deleted the move-from-storage-wrappers branch September 26, 2019 13:49
@rullzer
Copy link
Member

rullzer commented Sep 26, 2019

@icewind1991 backport?

@icewind1991
Copy link
Member Author

/backport to stable17

@icewind1991
Copy link
Member Author

/backport to stable16

@backportbot-nextcloud
Copy link

backport to stable17 in #17277

@backportbot-nextcloud
Copy link

backport to stable16 in #17278

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants