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

Define allowed app roots earlier #19039

Merged
merged 2 commits into from
Sep 15, 2015
Merged

Conversation

LukasReschke
Copy link
Member

The autoloader needs to be run before including the app.php, otherwise it depends on what app gets executed first and apps that rely on the dependency of other apps in app.php may break.

@LukasReschke LukasReschke added this to the 8.2-current milestone Sep 15, 2015
@LukasReschke
Copy link
Member Author

@cosenal @BernhardPosselt THX


// Add each app as allowed class path
foreach($apps as $app) {
\OC::$loader->addValidRoot(self::getAppPath($app));
Copy link
Contributor

Choose a reason for hiding this comment

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

Issue probably when the getter is called more than one time right?
I would not expect this from a getter :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Moved this to loadApps which should only be called once. That said, even if it is called twice the worst thing which can happen is to have the root allowed twice :)

@LukasReschke LukasReschke force-pushed the setup-autoloader-earlier branch 2 times, most recently from 2a7f7d7 to bac8c3e Compare September 15, 2015 10:04
@BernhardPosselt
Copy link
Contributor

Perfect :) 👍

The autoloader needs to be run before including the app.php, otherwise it depends on what app gets executed first and apps that rely on the dependency of other apps in app.php may break.
@LukasReschke LukasReschke force-pushed the setup-autoloader-earlier branch from bac8c3e to 8e1b403 Compare September 15, 2015 10:10
@cosenal
Copy link
Member

cosenal commented Sep 15, 2015

That solves my issue 👍

cosenal added a commit to cosenal/mailsharenewsplugin that referenced this pull request Sep 15, 2015
DeepDiver1975 added a commit that referenced this pull request Sep 15, 2015
@DeepDiver1975 DeepDiver1975 merged commit b6fe5b6 into master Sep 15, 2015
@DeepDiver1975 DeepDiver1975 deleted the setup-autoloader-earlier branch September 15, 2015 11:24
@RobinMcCorkell
Copy link
Member

This just (probably) broke upgrading apps, since the addValidRoot has been moved out of loadApp(). See #18839 (comment)

@DeepDiver1975
Copy link
Member

This just (probably) broke upgrading apps, since the addValidRoot has been moved out of loadApp(). See #18839 (comment)

can we please get a clear state on this? THX

@RobinMcCorkell
Copy link
Member

Phew, it seems we have just avoided disaster. loadApps() is called at least once during upgrades, so all enabled apps are properly loaded. However, we still have the problem that if some code somewhere tries to use loadApp() by itself, that app won't have its paths registered, so it will fail to load. Perhaps we should still have the addValidRoot() in loadApp() as well as in loadApps()?

@BernhardPosselt
Copy link
Contributor

@Xenopathic sounds like a plan. Would have to make sure to not add duplicates, but IIRC its a hashmap anyways, so it should not be a problem right?

@RobinMcCorkell
Copy link
Member

@BernhardPosselt Not a hash map, just a regular array. Adding duplicate entries will decrease performance by a small bit, so perhaps just changing it to a hashmap would be the easiest way to prevent duplicates.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 10, 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.

5 participants