NotificationsYou must be signed in to change notification settings
Fork4.1k
Star12.6k

Remove`StaticAssetsMetadata`#13094

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Draft

mkurz wants to merge4 commits intoplayframework:main

base:main

Choose a base branch

frommkurz:rm_staticassetsmetadata

Draft

Remove`StaticAssetsMetadata`#13094

mkurz wants to merge4 commits intoplayframework:mainfrommkurz:rm_staticassetsmetadata

Conversation

Copy link

Member

mkurz commentedJan 28, 2025•
edited
Loading

Currently, we have two approaches for reverse routing public assets, both are describedhere:

This pull requests removes the first one by removingStaticAssetsMetadata. Reason for removal is to remove all static/"global" state.

However I am not sure if we really should merge this pull request.
Reason is, the first approach is a bit easier for people how do not care about a static state in their applications, like when you are working on a single Play instance and not plan to run multiple Play apps within one jvm.

I think if we just keep both approaches documented it should be ok.

Not sure yet.

mkurz commented

Jan 28, 2025

View reviewed changes

core/play/src/main/scala/play/api/controllers/Assets.scala

		val path = base + "/" + value.name
		StaticAssetsMetadata.finder.findAssetPath(base, path)
		}
		}

Copy link

MemberAuthor

mkurzJan 28, 2025

Choose a reason for hiding this comment

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

This removal could cause problems I guess...

Copy link

Member

ihostageJan 28, 2025

Choose a reason for hiding this comment

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

You are absolutely right. This is binder for Assets for generated routes are like

GET            /public/*file                      controllers.Assets.versioned(path = "/public", file: controllers.Assets.Asset)

Copy link

MemberAuthor

mkurzFeb 17, 2025

Choose a reason for hiding this comment

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

Updated the description of the pull request. I could make the PR green by switching everything to assetsfinder.

Copy link

Member

ihostageFeb 17, 2025

Choose a reason for hiding this comment

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

If we will merge this, we also should remove the methodcontrollers.Assets.versioned(String, Asset) and for that we will should mark it as@Deprecated in2.9.x and3.0.x