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

fix: Correctly return app id and app version for core styles and images #50398

Merged
merged 2 commits into from
Jan 24, 2025
Merged

fix: Correctly return app id and app version for core styles and images #50398

merged 2 commits into from
Jan 24, 2025
+148 −39

Conversation

susnux
Copy link
Contributor

@susnux susnux commented Jan 24, 2025

Make sure that assets loaded from core/css and core/img are correctly assigned the server version.

@susnux susnux added bug 3. to review Waiting for reviews labels Jan 24, 2025
@susnux susnux mentioned this pull request Jan 24, 2025
Closed
@susnux susnux force-pushed the fix/get-version-of-core branch from 03e2dbe to 31664b3 Compare January 24, 2025 12:08
ChristophWurst
ChristophWurst approved these changes Jan 24, 2025
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.

Makes sense while it also feels a tiny bit wrong to treat core as an app. I'd favor adjusting the caller to not look for an app version for appid=core but query ServerVersion directly

@susnux
Copy link
Contributor Author

susnux commented Jan 24, 2025

Yes but better safe than sorry (we also have many other places that treat core as an app)

@ChristophWurst
Copy link
Member

Fair enough :shipit:

@susnux
Copy link
Contributor Author

susnux commented Jan 24, 2025

✅ Tests included

@susnux susnux force-pushed the fix/get-version-of-core branch from 026b8d7 to 3abb708 Compare January 24, 2025 13:35
juliusknorr
juliusknorr approved these changes Jan 24, 2025
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.

More complete then my quick patch and has tests 👍 ❤️

@susnux susnux force-pushed the fix/get-version-of-core branch from 3abb708 to eea91fa Compare January 24, 2025 14:10
@susnux susnux merged commit 6b4518c into master Jan 24, 2025
189 checks passed
@susnux susnux deleted the fix/get-version-of-core branch January 24, 2025 18:11
@susnux
Copy link
Contributor Author

susnux commented Jan 24, 2025

/backport to stable31

@susnux
Copy link
Contributor Author

susnux commented Jan 24, 2025

/backport to stable30

@backportbot backportbot bot mentioned this pull request Jan 24, 2025
Merged
This was referenced Jan 24, 2025
Draft
Draft
@AndyScherzinger AndyScherzinger added this to the Nextcloud 32 milestone Jan 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@juliusknorr juliusknorr juliusknorr approved these changes

@ChristophWurst ChristophWurst ChristophWurst approved these changes

@AndyScherzinger AndyScherzinger AndyScherzinger approved these changes

@nickvergessen nickvergessen Awaiting requested review from nickvergessen

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

Successfully merging this pull request may close these issues.
4 participants
@susnux @ChristophWurst @AndyScherzinger @juliusknorr