-
Notifications
You must be signed in to change notification settings - Fork 28
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
Update TagLib URIs for EE10 #172
Conversation
JSP Root URI
XPath
Schema Namespaces for TLDs
Examples
permittedTaglibs.tld
Other TLDs
I don't see why we should keep the older ones if they aren't used (unless a developer overrides them?) |
7a043c6
to
738bcff
Compare
I think this is ready for review. |
34550cd
to
88d9fac
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Vlad, I'd like to also see this one added to the changelog in the spec document.
88d9fac
to
5a88a5f
Compare
@@ -42,7 +42,7 @@ | |||
<display-name>permittedTaglibs</display-name> | |||
<tlib-version>1.1</tlib-version> | |||
<short-name>permittedTaglibs</short-name> | |||
<uri>http://java.sun.com/jstl/permittedTaglibs</uri> | |||
<uri>http://jakarta.apache.org/taglibs/standard/permittedTaglibs</uri> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the example to use this URI instead. Taken from the permittedTaglibs.tld : https://github.com/eclipse-ee4j/jstl-api/blob/master/impl/src/main/resources/META-INF/permittedTaglibs.tld#L30
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" | ||
xsi:schemaLocation= + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good job cleaning this up!
The current changes look good. I would like us to do some investigation and have some discussions to determine if we need to or if we should support both for existing applications. Can we do that before we merge this so we know the full scope of this work and can open issues as necessary @volosied ? I'll hold off on approving until we determine what if anything we need to do here in addition to the current PR. |
@arjantijms @pnicolucci Before I merge, do you think we should support the old URIs ( Edit: May through a property, at least? TLD is associated with one URI, so we might need to have more TLD files (for EE8 / EE9 cutoff) The property would enable use of the older TLDs (and therefore the older URIs)? |
A property would not hurt much. It does bloat the code, but we could deprecate it next version and then remove the version after or so. The support for these old TLD files could maybe be done in a separate PR? |
5a88a5f
to
1dc349e
Compare
@arjantijms @pnicolucci I think a property is a good idea. I've created #193, and I'll merge this PR for now. |
Update Spec Doc & Update package.html
1dc349e
to
7e85287
Compare
Squashed commits. Merging. |
for issue #144
TLD docs list the new URIs as so:
<%@ taglib prefix="c" uri="jakarta.tags.core" %>
I tested the new implementation on Tomcat 10.0.12, and the new URIs work well. If the old ones were used, then the server encountered the following error:
Questions:
Should there be backwards compatibility with the new and old URIs?
JSP Root URI
Should this change? Do we need to wait for the Pages-Api project?
https://github.com/eclipse-ee4j/jstl-api/blob/2a60c5505d3466c49e8f4ab1e49ef42ec0e5e7a3/api/src/main/java/jakarta/servlet/jsp/jstl/tlv/PermittedTaglibsTLV.java#L63
Other TLDs
What should we do with the older TLDs? They use "http://java.sun.com/jstl/" instead of "http://java.sun.com/jsp/jstl/"
https://github.com/eclipse-ee4j/jstl-api/blob/2a60c5505d3466c49e8f4ab1e49ef42ec0e5e7a3/impl/src/main/resources/META-INF/sql-1_0.tld#L27
XPath
Not sure what this is about. Seems safe to change?
https://github.com/eclipse-ee4j/jstl-api/blob/2a60c5505d3466c49e8f4ab1e49ef42ec0e5e7a3/impl/src/main/java/org/apache/taglibs/standard/tag/common/xml/JSTLXPathImpl.java#L188
XPath
Not sure where the
http://java.sun.com/jstl/xpath
namespace is used or how? Could just be local in this impl?https://github.com/eclipse-ee4j/jstl-api/blob/2a60c5505d3466c49e8f4ab1e49ef42ec0e5e7a3/impl/src/main/java/org/apache/taglibs/standard/tag/common/xml/JSTLXPathVariableResolver.java#L40-L43
Schema Namespaces for TLDs
Still waiting for new EE 10 schemas: https://jakarta.ee/xml/ns/jakartaee/#10 Only a few are out and they are still drafts.
https://github.com/eclipse-ee4j/jstl-api/blob/2a60c5505d3466c49e8f4ab1e49ef42ec0e5e7a3/impl/src/main/resources/META-INF/c.tld#L20-L23
Using EE9 schemas give me the following error:
Should we update the example?
https://github.com/eclipse-ee4j/jstl-api/blame/2a60c5505d3466c49e8f4ab1e49ef42ec0e5e7a3/spec/src/main/asciidoc/jakarta-stl.adoc#L7225-L7255
This should also be updated?
https://github.com/eclipse-ee4j/jstl-api/blob/master/api/src/main/java/jakarta/servlet/jsp/jstl/tlv/package.html
Other URIs that need to be updated? Such as
https://github.com/eclipse-ee4j/jstl-api/blob/2a60c5505d3466c49e8f4ab1e49ef42ec0e5e7a3/impl/src/main/resources/META-INF/permittedTaglibs.tld#L30