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

Add a separate event for login page rendering #39867

Merged
merged 5 commits into from
Aug 18, 2023
Merged

Conversation

icewind1991
Copy link
Member

@icewind1991 icewind1991 commented Aug 14, 2023

Only very few apps actually need to add their scripts/styles to the login page, but it's currently easy for apps to accidentally do that anyway.

By having a separate event, having your scripts on the login page becomes opt-in rather than opt-out.

  • Add a separate event for login page rendering
  • Move some more event loading logic to the correct events.
  • test that nothing explodes?

@icewind1991 icewind1991 added this to the Nextcloud 28 milestone Aug 14, 2023
@icewind1991 icewind1991 force-pushed the login-less branch 3 times, most recently from 1facc8c to 40cf409 Compare August 15, 2023 08:07
@icewind1991
Copy link
Member Author

https://github.com/nextcloud/unsplash/blob/4c04d275e9e2469c5b6f1b3d2b6f24cfe9db9887/lib/EventListener/BeforeTemplateRenderedEventListener.php#L18 is the only non-core app I could find in a quick search that would need to be adjusted

@icewind1991
Copy link
Member Author

  • 2fa apps have their own templates to do script/style adding with
  • register app uses the "alternative login" feature instead of events
  • user_saml doesn't seem to rely on script injection

Seems safe to me to apply this change.

@icewind1991 icewind1991 marked this pull request as ready for review August 17, 2023 08:50
@icewind1991 icewind1991 added the 2. developing Work in progress label Aug 17, 2023
@icewind1991 icewind1991 mentioned this pull request Aug 17, 2023
@icewind1991 icewind1991 added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Aug 17, 2023
@icewind1991 icewind1991 requested review from a team, ArtificialOwl, blizzz and nfebe and removed request for a team August 17, 2023 17:30
@icewind1991 icewind1991 added the pending documentation This pull request needs an associated documentation update label Aug 18, 2023
@nickvergessen nickvergessen merged commit be41dc4 into master Aug 18, 2023
39 checks passed
@nickvergessen nickvergessen deleted the login-less branch August 18, 2023 11:54
@ChristophWurst
Copy link
Member

image

breaks the installer styling and scripts.

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.

The new event is not emitted

@icewind1991
Copy link
Member Author

The downside of having your dev setup perform the setup automatically 🙈

@icewind1991
Copy link
Member Author

#40494

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews enhancement pending documentation This pull request needs an associated documentation update performance 🚀
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants