jakartaee / faces

Jakarta Faces
Other
109 stars 55 forks source link

Introduce FacesServletFactory #1861

Closed mnriem closed 11 months ago

mnriem commented 1 year ago

To facilitate different implementors the FacesServlet should delegate to a Servlet that should be acquired by the FacesServletFactory.

tandraschko commented 1 year ago

whats the usecase for it?

mnriem commented 1 year ago

Rought implementation details

  1. Move code in FacesServlet to a vendor specific implementation in the vendor implementation project.
  2. Change the FacesServlet in the API to use the FacesServletFactory to get the Servlet instance it will delegate to (which should be the specific implementation as created in 1.)
tandraschko commented 1 year ago

Currently this isnt alligned with other Factories: class ViewDeclarationLanguageFactory implements FacesWrapper<ViewDeclarationLanguageFactory> {

tandraschko commented 1 year ago

any reason for this?

BalusC commented 1 year ago

Currently this isnt alligned with other Factories: class ViewDeclarationLanguageFactory implements FacesWrapper<ViewDeclarationLanguageFactory> {

Oh this reminds me we still need to add to spec that all factories and factory instances should preferably implement FacesWrapper and that impl should validate that by e.g. emitting warnings. Or perhaps even make it obligatory.

any reason for this?

what exactly are you referring to?

tandraschko commented 1 year ago

what exactly are you referring to?

that its implemented without FacesWrapper :)

also we should add faces-config/factory/faces-servlet-factory like other factories before

tandraschko commented 1 year ago

What about the faces-config.xml Changes?

mnriem commented 1 year ago

@BalusC @arjantijms Should the latest faces-config XSD be included in the API JAR going forward?

tandraschko commented 11 months ago

getFacesServlet is still unused in Mojarra? where do you use it?

In MyFaces we do this: servletContext.addServlet(FACES_SERVLET_NAME, FacesServlet.class);

in this case, the FacesServletFactory looks a bit wrong? public Servlet getFacesServlet(ServletConfig config) should be public Class<? extends FacesServlet> getFacesServlet() ?

mnriem commented 11 months ago

@tandraschko The API is correct. The FacesServletFactory is supposed to deliver an instance of the Servlet that the FacesServlet can delegate to.

tandraschko commented 11 months ago

Implemented it in MF now: https://github.com/apache/myfaces/commit/3a9e428386a974046fd30bdc0b19c1ba999d5b20 please review