Cleanup webappconfiguration #3703
Conversation
Signed-off-by: Greg Wilkins <gregw@webtide.com>
Signed-off-by: Greg Wilkins <gregw@webtide.com>
…configuration-cleanup Signed-off-by: Greg Wilkins <gregw@webtide.com>
Signed-off-by: Greg Wilkins <gregw@webtide.com>
examples/async-rest/async-rest-webapp/src/main/webapp/WEB-INF/jetty-web.xml
Show resolved
Hide resolved
|
Note that this PR is really a draft until we work out how to fix the OSGi tests. @janbartel any ideas yet? |
|
@janbartel nudge |
…configuration-cleanup
Signed-off-by: Greg Wilkins <gregw@webtide.com>
gregw
force-pushed
the
jetty-10.0.x-configuration-cleanup
branch
from
34ad5f6
to
52c332a
Compare
| "org.eclipse.jetty.alpn."); | ||
| "org.eclipse.jetty.servlet.NoJspServlet" | ||
| ); | ||
| expose("org.eclipse.jetty.servlet.listener."); |
There was a problem hiding this comment.
All these are classes/packages are in org.eclipse.jetty.servlet.
Would it not make more sense to have a Configuration class in the jetty-servlet module rather than in jetty-webapp?
There was a problem hiding this comment.
Not really. servlet is usable without webapps. But configuration really about configuring webapps - specifically by a combination of convention and the configurations we push into it from our modules. If we wanted to move Configuration down to servlet, then we'd have to move the WebAppClassLoader (or at least an abstraction of it) down as well... and then we start having no difference between servlet and webapp.
tl;dr; any direct users of jetty-servlet configure by using API directly and probably don't have a different classloader (maybe not even a context).
examples/async-rest/async-rest-webapp/src/main/webapp/WEB-INF/jetty-web.xml
Show resolved
Hide resolved
| @@ -141,6 +141,7 @@ | |||
| <groupId>org.eclipse.jetty</groupId> | |||
| <artifactId>jetty-servlets</artifactId> | |||
| <version>${project.version}</version> | |||
| <scope>provided</scope> | |||
There was a problem hiding this comment.
Why provided? jetty-servlets is supposed to be portable across containers (it's not, but I think that's the idea), so it should really be in WEB-INF/lib.
There was a problem hiding this comment.
I don't think they are portable. More over, the jetty-servlets module exposes them on the webapp classpath. Putting them in webapp just brings in all our io, http and util classes and exposes all the classloader problems that entails.
In fact, some of the classes (eg PushCacheFilter) only work if loaded from the container. I don't think we are in the business of providing general utility servlets for any container.
| break; | ||
| out.write(buffer,0,len); | ||
| } | ||
| } |
There was a problem hiding this comment.
What's the reason to inline IO.copy()?
There was a problem hiding this comment.
because you wouldn't let me do a release of a fixed jetty-util jar :)
Once we have a fixed 9.4 jetty-util release, we can re-instate the tests that use it and then we can remove this inline (I told you it was a pain!)
I don't want to bring in jetty-10 util just for this 1 usage.
tests/test-webapps/test-servlet-spec/test-spec-webapp/src/main/webapp/WEB-INF/jetty-web.xml
Show resolved
Hide resolved
Signed-off-by: Greg Wilkins <gregw@webtide.com>
cleanup webappconfiguration