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

allow routes to specify that they dont need the filesystem to be setup in advance #1233

Closed
wants to merge 1 commit into from

Conversation

icewind1991
Copy link
Member

This way we don't have to do an expensive filesystem setup for requests that don't need it.

Note that this doesn't mean that the route can't use the filesystem, if a route gets the root folder from the server container it will still automatically setup the fs once it's used or the route can manually setup the fs if it wants to use the older api's

We can't enable this by default for legacy compatibility

cc @rullzer

@icewind1991 icewind1991 added the 3. to review Waiting for reviews label Sep 1, 2016
@icewind1991 icewind1991 added this to the Nextcloud 11.0 milestone Sep 1, 2016
/**
* Set whether or not the filesystem should be setup to handle this route
*
* @param $filesystem
Copy link
Member

Choose a reason for hiding this comment

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

type?!

@nickvergessen
Copy link
Member

The theming images are stored in the data dir, but I guess that does not need the filesystem to be set up?

@icewind1991
Copy link
Member Author

The theming images are stored in the data dir, but I guess that does not need the filesystem to be set up?

Since that uses the RootFolder as entrypoint to the filesystem the filesystem is automatically setup when used

@rullzer
Copy link
Member

rullzer commented Sep 1, 2016

Awesome idea @icewind1991! Especially now that the rootfolder is always lazy!

@rullzer
Copy link
Member

rullzer commented Sep 10, 2016

@icewind1991 I still don't think we should put this kind of info in the routes.
Routes are our way to tell where to look. They should not influence behaviour.

So the constructor is basically called at https://github.com/nextcloud/server/blob/master/lib/private/AppFramework/App.php#L114

And then only in https://github.com/nextcloud/server/blob/master/lib/private/AppFramework/App.php#L127 the first middleware is called.

So for routes that use the appframework we could easily add a a middleware that checks for the 'NoFS' annoation. Of course for routes that do not use the appframework we will have to call it still.

I'm not fully aware of the implication of not calling setupFS. But most appFramework stuff doesn't use it right? Or else it is small enough that we can fix it. I mean apps should not use the view anyways. And if they do we can tell them run setupFS first right?

@nickvergessen
Copy link
Member

I mean apps should not use the view anyways. And if they do we can tell them run setupFS first right?

View is the API apps use, don't argue with "should not"

@rullzer
Copy link
Member

rullzer commented Sep 12, 2016

@nickvergessen why not? We have OCP for a reason. If devs decide to run stuff from our private namespace they can expect it to break. The node API has been around for a very long time already.

@nickvergessen
Copy link
Member

@nickvergessen why not? We have OCP for a reason. If devs decide to run stuff from our private namespace they can expect it to break. The node API has been around for a very long time already.

View is defacto public API, since there was no other way in the past (Nodes API is quite new). Yes, nodes should be used, but for an discussion about routes and breaking apps, view is to be considered public api

@rullzer
Copy link
Member

rullzer commented Sep 12, 2016

It has been introduced almost 3 years ago. Honestly at some point we have to say tough luck.

But ok. Then 👎 from me. We should not hack in such stuff in the routing

@nickvergessen nickvergessen added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Sep 26, 2016
@rullzer
Copy link
Member

rullzer commented Nov 2, 2016

Lets close this for now as there is no way to do this cleanly without breaking some stuff right now. See #1987

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

Successfully merging this pull request may close these issues.

None yet

4 participants