Skip to content

Comments

chore: Remove useless legacy autoloader for tests#52945

Merged
skjnldsv merged 1 commit intomasterfrom
fix/cleanup-test-legacy-autoloader
May 27, 2025
Merged

chore: Remove useless legacy autoloader for tests#52945
skjnldsv merged 1 commit intomasterfrom
fix/cleanup-test-legacy-autoloader

Conversation

@come-nc
Copy link
Contributor

@come-nc come-nc commented May 19, 2025

Summary

We do not need the legacy autoloader for the TestCase, because we have PSR-4 loading added in 54250ed

Checklist

@come-nc come-nc added this to the Nextcloud 32 milestone May 19, 2025
@come-nc come-nc self-assigned this May 19, 2025
@come-nc come-nc added the 3. to review Waiting for reviews label May 19, 2025
@come-nc come-nc force-pushed the fix/cleanup-test-legacy-autoloader branch 2 times, most recently from d17c564 to 9243305 Compare May 19, 2025 08:03
@come-nc come-nc marked this pull request as ready for review May 19, 2025 08:03
@come-nc come-nc requested a review from a team as a code owner May 19, 2025 08:03
@come-nc come-nc requested review from ArtificialOwl, skjnldsv and sorbaugh and removed request for a team May 19, 2025 08:03
@come-nc come-nc mentioned this pull request May 19, 2025
Merged
4 tasks
nickvergessen
nickvergessen requested changes May 19, 2025
View reviewed changes
Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

This breaks apps

@come-nc
Copy link
Contributor Author

come-nc commented May 19, 2025

This breaks apps

I expected apps were using tests/bootstrap.php, but it seems a lot have their own.

They should be updated to use:

\OC::$composerAutoloader->addPsr4('Test\\', OC::$SERVERROOT . '/tests/lib/', true);
\OC::$composerAutoloader->addPsr4('Tests\\', OC::$SERVERROOT . '/tests/', true);

As is done is server bootstrap.php.

That is still not that pretty as it depends upon \OC::$composerAutoloader which should not be public API.
Not sure what a pretty solution would be?
Maybe we add an autoload.php in tests/ that application can require?

@come-nc come-nc mentioned this pull request May 19, 2025
Merged
4 tasks
@come-nc
Copy link
Contributor Author

come-nc commented May 19, 2025 •
edited
Loading

I extracted the autoload.php into #52951

This was referenced May 19, 2025
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
@nickvergessen nickvergessen mentioned this pull request May 20, 2025
Merged
This was referenced May 20, 2025
Merged
Merged
This was referenced May 20, 2025
Merged
Merged
Merged
Merged
Merged
skjnldsv
skjnldsv approved these changes May 21, 2025
View reviewed changes
Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

Once apps are handled: 👌

@nickvergessen
Copy link
Member

nickvergessen commented May 22, 2025

What about all the community apps?
https://github.com/search?q=org%3Anextcloud%20addValidRoot&type=code

@skjnldsv
Copy link
Member

skjnldsv commented May 22, 2025

What about all the community apps? github.com/search?q=org%3Anextcloud%20addValidRoot&type=code

communicate as breaking and move on ?

@nickvergessen nickvergessen added the pending documentation This pull request needs an associated documentation update label May 22, 2025
This was referenced May 26, 2025
Merged
Merged
Merged
@come-nc come-nc force-pushed the fix/cleanup-test-legacy-autoloader branch from d96db5c to 9560e00 Compare May 27, 2025 14:16
@come-nc
Copy link
Contributor Author

come-nc commented May 27, 2025

@nickvergessen Can we merge now?
There is a workaround with tests/bootstrap.php and most apps migrated to it.

@skjnldsv skjnldsv merged commit c3f16a5 into master May 27, 2025
229 of 240 checks passed
@skjnldsv skjnldsv deleted the fix/cleanup-test-legacy-autoloader branch May 27, 2025 15:57
@icewind1991 icewind1991 mentioned this pull request May 28, 2025
Merged
@provokateurin provokateurin mentioned this pull request Jun 2, 2025
Merged
st3iny added a commit to nextcloud/twofactor_webauthn that referenced this pull request Jun 9, 2025
@st3iny
chore(tests): cleanup bootstrap.php to be forward-compatible
5041440 
Ref nextcloud/server#52945

Signed-off-by: Richard Steinmetz <[email protected]>
@st3iny st3iny mentioned this pull request Jun 9, 2025
Merged
This was referenced Jun 10, 2025
Merged
Merged
@skjnldsv skjnldsv mentioned this pull request Aug 19, 2025
Merged
@skjnldsv skjnldsv modified the milestones: Nextcloud 32, Nextcloud 33 Sep 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nickvergessen nickvergessen nickvergessen approved these changes

@skjnldsv skjnldsv skjnldsv approved these changes

@ChristophWurst ChristophWurst Awaiting requested review from ChristophWurst

@susnux susnux Awaiting requested review from susnux

@ArtificialOwl ArtificialOwl Awaiting requested review from ArtificialOwl ArtificialOwl is a code owner automatically assigned from nextcloud/server-backend

@sorbaugh sorbaugh Awaiting requested review from sorbaugh sorbaugh is a code owner automatically assigned from nextcloud/server-backend

Assignees

@come-nc come-nc

Labels

3. to review Waiting for reviews pending documentation This pull request needs an associated documentation update

Projects

None yet

Milestone

Nextcloud 32

Development

Successfully merging this pull request may close these issues.

3 participants

@come-nc @nickvergessen @skjnldsv