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

[WIP] Cleanup filesystem setup code #11091

Closed
wants to merge 3 commits into from
Closed

Conversation

icewind1991
Copy link
Contributor

  • Move the logic of setting up the filesystem from "all over the place" to \OC\Files\Factory
  • Seperate out logic of setting up the filesystem for object storage based systems (cc @butonic)
  • Remove filesystem state from \OC\Files\Filesystem
  • Let the server container keep track of the state instead

cc @DeepDiver1975 @PVince81 @th3fallen

@RobinMcCorkell
Copy link
Member

cc myself

@PVince81
Copy link
Contributor

I was hoping that at some point we could reach a level where setting up the FS isn't something global/static (which here is still the case even when stored in \OC::$server).

Ideally it should be possible to call setupFS() and get an instance of filesystem for that user, making it possible to have FS instances from multiple users (for example for sharing).

Will this change somehow bring us closer to that ?

@icewind1991
Copy link
Contributor Author

Some state needs to be global in some way, having multiple instances of storage objects for the "same" storage will lead to problems.

The user specific state should be removed yet, ideally any filesystem calls will be independent of the logged in user.

I'll see if some changes can be made to this PR in that direction

@PVince81
Copy link
Contributor

How about having a "pool" of storage instances, then ? 😄

I'm not expecting you to add changes just yet, just see whether this PR is somehow compatible with that vision 😄

@icewind1991
Copy link
Contributor Author

I'm not expecting you to add changes just yet, just see whether this PR is somehow compatible with that vision 😄

I know, just making sure this PR doesn't add anything that will make this more difficult in the future

@icewind1991 icewind1991 force-pushed the filesystem-factory branch 2 times, most recently from f8f3889 to 79843c8 Compare November 7, 2014 14:22
@butonic
Copy link
Member

butonic commented Nov 11, 2014

@icewind1991 is this ready to review or WIP?

@icewind1991
Copy link
Contributor Author

There are a few failing test cases that need to be resolved but other than that it's ready for review

@icewind1991 icewind1991 force-pushed the filesystem-factory branch 2 times, most recently from c6a85dd to b3265aa Compare November 26, 2014 13:31
@PVince81
Copy link
Contributor

More a refactor than a simply cleanup 😉

}

/**
* Mount the storages for a user
Copy link
Contributor

Choose a reason for hiding this comment

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

Make it clearer

Copy link
Contributor

Choose a reason for hiding this comment

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

"Returns a folder with the point of view of the given user, with all mount points applied"


namespace OC\Files;

class FileInfoManager {
Copy link
Contributor

Choose a reason for hiding this comment

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

Double check whether it's still needed ?

$this->registerService('FilesystemFactory', function (Server $c) {
\OC_App::loadApps(array('filesystem'));
$config = $c->getConfig();
if ($config->getSystemValue('objectstore', false)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be moved into a separate factory so you can test it, much like https://github.com/owncloud/news/blob/master/db/mapperfactory.php

@LukasReschke LukasReschke changed the title Cleanup filesystem setup code [WIP] Cleanup filesystem setup code Feb 24, 2015
@PVince81
Copy link
Contributor

PVince81 commented Jul 3, 2015

@icewind1991 any update ? Is that the one you are splitting in smaller pieces ?

@PVince81
Copy link
Contributor

PVince81 commented Sep 1, 2015

Needs rebasing.

Didn't you split this up already partly ?

@icewind1991
Copy link
Contributor Author

Some of it went in the various other filesystem related PR over the year, but most of it still needs to be done

@MorrisJobke
Copy link
Contributor

This PR is over an year old and touches a lot of files that have changed meanwhile. Can this be splitted up in smaller parts to make it easier to review or what is the state of this?

@icewind1991
Copy link
Contributor Author

Yes, would be better to close this and make new PR(s) when we decide to work on it further

Trying to rebase/update this PR would probably be more work then redoing it

@MorrisJobke MorrisJobke deleted the filesystem-factory branch November 23, 2015 13:07
@lock lock bot locked as resolved and limited conversation to collaborators Aug 9, 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.

8 participants