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.

Sign up for GitHub

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

Jump to bottom

Do not load app.php if Application implements IBootstrap #21812

Merged
merged 1 commit into from
Jul 14, 2020
Merged

Do not load app.php if Application implements IBootstrap #21812

merged 1 commit into from
Jul 14, 2020

Conversation

MorrisJobke
Copy link
Member

The problem with this is that it changes the load order also for apps without the IBootstrap. Another approach would be a big warning if the app is then one with IBootstrap but still has an app.php.

#20573 and #20865

@MorrisJobke MorrisJobke added enhancement 3. to review Waiting for reviews labels Jul 13, 2020
@MorrisJobke MorrisJobke added this to the Nextcloud 20 milestone Jul 13, 2020
@MorrisJobke MorrisJobke force-pushed the feature/noid/app-bootstrap-without-app.php branch from cbf0925 to 7d3c308 Compare July 14, 2020 03:04
@ChristophWurst
Copy link
Member

The problem with this is that it changes the load order also for apps without the IBootstrap

Why would it do that?

@MorrisJobke
Copy link
Member Author

Why would it do that?

Because I moved the IBootstrap call to bootApp above the include of the app.php. Thus the Application is loaded before the app.php. In the past it was the other way around. Sometimes this matter (if the autoloader is configured in the app.php vor example)

ChristophWurst
ChristophWurst reviewed Jul 14, 2020
View reviewed changes
lib/private/legacy/OC_App.php Outdated Show resolved Hide resolved
lib/private/legacy/OC_App.php Show resolved Hide resolved
@MorrisJobke MorrisJobke force-pushed the feature/noid/app-bootstrap-without-app.php branch from 7d3c308 to d6339e7 Compare July 14, 2020 12:23
@MorrisJobke
Copy link
Member Author

Ready for review :)

ChristophWurst
ChristophWurst approved these changes Jul 14, 2020
View reviewed changes
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.

Looks great but I'd appreciate a more sane naming of this variable 🤡

lib/private/legacy/OC_App.php Outdated Show resolved Hide resolved
@MorrisJobke
Do not load app.php if Application implements IBootstrap
d9cc2f0 
Signed-off-by: Morris Jobke <hey@morrisjobke.de>
@MorrisJobke MorrisJobke added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Jul 14, 2020
@MorrisJobke MorrisJobke force-pushed the feature/noid/app-bootstrap-without-app.php branch from d6339e7 to d9cc2f0 Compare July 14, 2020 13:56
@MorrisJobke MorrisJobke merged commit fba64a9 into master Jul 14, 2020
@MorrisJobke MorrisJobke deleted the feature/noid/app-bootstrap-without-app.php branch July 14, 2020 15:06
@ChristophWurst ChristophWurst mentioned this pull request Jul 14, 2020
Closed
@Grotax Grotax mentioned this pull request Sep 25, 2020
Closed
@icewind1991 icewind1991 mentioned this pull request Oct 1, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChristophWurst ChristophWurst ChristophWurst approved these changes

@skjnldsv skjnldsv skjnldsv approved these changes

@rullzer rullzer Awaiting requested review from rullzer

@kesselb kesselb Awaiting requested review from kesselb

@nickvergessen nickvergessen Awaiting requested review from nickvergessen

@juliusknorr juliusknorr Awaiting requested review from juliusknorr

Assignees
No one assigned
Labels
4. to release Ready to be released and/or waiting for tests to finish enhancement
Projects
None yet
Milestone
Nextcloud 20
Development

Successfully merging this pull request may close these issues.
3 participants
@MorrisJobke @ChristophWurst @skjnldsv