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

also load app autoloader when located in /vendor #23125

Closed
wants to merge 1 commit into from
Closed

also load app autoloader when located in /vendor #23125

wants to merge 1 commit into from

Conversation

icewind1991
Copy link
Member

@icewind1991 icewind1991 commented Oct 1, 2020 •
edited
Loading

with the phasing out of app.php apps can no longer load the autoloader from there themselves

See #21812

@icewind1991
also load app autoloader when located in /vendor
e062c42 
Signed-off-by: Robin Appelman <robin@icewind.nl>
@icewind1991 icewind1991 added the 3. to review Waiting for reviews label Oct 1, 2020
@icewind1991 icewind1991 added this to the Nextcloud 21 milestone Oct 1, 2020
@icewind1991
Copy link
Member Author

/backport to stable20

@faily-bot
Copy link

faily-bot bot commented Oct 1, 2020

🤖 beep boop beep 🤖

Here are the logs for the failed build:

Status of 33536: failure

jsunit

Show full log 
PhantomJS 2.1.1 (Linux 0.0.0) WARN: 'No OC found'
PhantomJS 2.1.1 (Linux 0.0.0) LOG: 'JQMIGRATE: Migrate is installed, version 1.4.1'
PhantomJS 2.1.1 (Linux 0.0.0) WARN: 'jQuery is deprecated: The global jQuery is deprecated. It will be updated to v3.x in Nextcloud 21. In later versions of Nextcloud it might be removed completely. Please ship your own.'
PhantomJS 2.1.1 (Linux 0.0.0) WARN: '$ is deprecated: The global jQuery is deprecated. It will be updated to v3.x in Nextcloud 21. In later versions of Nextcloud it might be removed completely. Please ship your own.'
PhantomJS 2.1.1 (Linux 0.0.0) WARN: 'Handlebars is deprecated: please ship your own, this will be removed in Nextcloud 20'
PhantomJS 2.1.1 (Linux 0.0.0) DEBUG: 'OCA.Files.Settings initialized'
PhantomJS 2.1.1 (Linux 0.0.0) DEBUG: 'OCA.Files.Sidebar initialized'
PhantomJS 2.1.1 (Linux 0.0.0) DEBUG: 'OCA.Sharing.ShareSearch initialized'
PhantomJS 2.1.1 (Linux 0.0.0) DEBUG: 'OCA.Sharing.ExternalLinkActions initialized'
PhantomJS 2.1.1 (Linux 0.0.0): Executed 847 of 850 (skipped 3) SUCCESS (11.135 secs / 10.562 secs)

@ChristophWurst
Copy link
Member

with the phasing out of app.php apps can no longer load the autoloader from there themselves

But you can require_once from the header of Application.php, no?

@MorrisJobke
Copy link
Member

with the phasing out of app.php apps can no longer load the autoloader from there themselves

But you can require_once from the header of Application.php, no?

Or just put it in composer/autoloader.php?

juliusknorr
juliusknorr approved these changes Oct 6, 2020
View reviewed changes
Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

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

👍 from my side since vendor is actually the default target directory for installing composer packages without additional configuration of the vendor-dir

@ChristophWurst ChristophWurst added the pending documentation This pull request needs an associated documentation update label Nov 10, 2020
ChristophWurst
ChristophWurst approved these changes Nov 10, 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.

well fine I guess

@ChristophWurst
Copy link
Member

argh I'm so torn on this one. we have to check the file existence for every app. the os/fs will cache the result but still we do this. and if the require_once is in application.php (and we properly document this) it's explicit and less magical.

@MorrisJobke
Copy link
Member

and if the require_once is in application.php (and we properly document this) it's explicit and less magical.

Sounds sane 👍

ChristophWurst
ChristophWurst requested changes Nov 10, 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.

Then I'll change my mind one more time. I'll do the docs update.

This was referenced Nov 24, 2020
Merged
Merged
This was referenced Dec 14, 2020
Merged
Merged
This was referenced Dec 28, 2020
Merged
Merged
@rullzer rullzer modified the milestones: Nextcloud 21, Nextcloud 22 Jan 6, 2021
@icewind1991 icewind1991 deleted the autoload-app-vendor branch March 26, 2021 14:27
@ChristophWurst ChristophWurst removed the pending documentation This pull request needs an associated documentation update label Jul 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChristophWurst ChristophWurst ChristophWurst requested changes

@juliusknorr juliusknorr juliusknorr approved these changes

@rullzer rullzer Awaiting requested review from rullzer

@MorrisJobke MorrisJobke Awaiting requested review from MorrisJobke

Assignees
No one assigned
Labels
3. to review Waiting for reviews
Projects
None yet
Milestone
Nextcloud 22
Development

Successfully merging this pull request may close these issues.
5 participants
@icewind1991 @ChristophWurst @MorrisJobke @juliusknorr @rullzer