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

ResourceHandler set base resource as string #8735

Merged
merged 5 commits into from
Oct 20, 2022

Conversation

gregw
Copy link
Contributor

@gregw gregw commented Oct 18, 2022

Added convenience method to set resource from a string. Closes #8727.

@gregw gregw requested a review from sbordet October 18, 2022 21:59
@gregw gregw linked an issue Oct 18, 2022 that may be closed by this pull request
gregw added 4 commits October 20, 2022 07:43
 + Fix XML names
 + fix the anti-pattern of setBaseResourceAsPath(someResource.getPath())
Copy link
Contributor

@joakime joakime left a comment

Choose a reason for hiding this comment

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

Lots of mechanical changes.
Great cleanup of the Jetty XMLs. (even in the commented out projects/modules)
One odd deprecation. (IMO, just delete it)

I dismissed the CodeQL alert on Zip Slip (as that code is protected already)

*/
@ManagedAttribute("document root for context")
@Deprecated
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we just delete this method now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should keep it in ee9 as we are trying to be as compatible as possible with jetty-10.
We can remove in 12.1.0

Copy link
Contributor

@joakime joakime Oct 20, 2022

Choose a reason for hiding this comment

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

We already changed the contract from jetty-10 though.

Resource.newResource(String) no longer exists.
Have to use ResourceFactory.newResource(String) now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, there are changes, but it costs us nothing much to hold onto a deprecated method for at least a few release cycles.
We could remove it, but it can be a confusing name change, so leaving it as a sign post is a good idea.

@gregw gregw merged commit ac6abb3 into jetty-12.0.x Oct 20, 2022
@gregw gregw deleted the jetty-12-8727-setBaseResource-string branch October 21, 2022 01:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Jetty 12 - Improve usability for ResourceHandler
2 participants