Bug 54239

Created attachment 29685 [details]
ELInterpreterFactory

Created attachment 29686 [details]
Patch for Generator.java

Comment on attachment 29685 [details]
ELInterpreterFactory

In principle this looks like a good idea.

I have a couple of concerns with the patch as currently written:
1. No documentation.
2. No test cases.
3. The use of enum for the default instance is rather odd.
4. I dislike the use of system properties when they are not necessary. If the class name was handled as a servlet context initialization parameters then Tomcat already has the necessary plumbing for global, per host and per web application configuration.
5. Error messages need to use the standard i18n support.

Thanks Mark.

I'll provide document & test cases for BUG 54239-54242.

Created attachment 29693 [details]
Interface of ELInterpreter

Add more comments

Created attachment 29694 [details]
ELInterpreterFactory

Added more comments

Created attachment 29695 [details]
Patch for Generator.java

Patch for Generator.java & LocalStrings.*

Created attachment 29696 [details]
Test case for ELInterpreterFactory

Test case for ELInterpreterFactory

(In reply to comment #3)
> Comment on attachment 29685 [details]
> ELInterpreterFactory
> 
> In principle this looks like a good idea.
> 
> I have a couple of concerns with the patch as currently written:
> 1. No documentation.
     ~~~~~~~~~~~~~~~~~ Added more comments in ELInterpreter & ELInterpreterFactory
> 2. No test cases.
     ~~~~~~~~~~~~~~~~~ Provided a test case for ELInterpreterFactory

> 3. The use of enum for the default instance is rather odd.
     ~~~~~~~~~~~~~~~~~ Fixed

> 4. I dislike the use of system properties when they are not necessary. If
> the class name was handled as a servlet context initialization parameters
> then Tomcat already has the necessary plumbing for global, per host and per
> web application configuration.
     ~~~~~~~~~~~~~~~~~~ Supported passing the class name from context initialization parameters. Still keep the feature from System Property. 
     In our case, System Property can be easily passed in from such kind of Daemon-Watcher, For example: JSW.

> 5. Error messages need to use the standard i18n support.
     ~~~~~~~~~~~~~~~~~~ Fixed

(In reply to comment #9)
> (In reply to comment #3)

> > 4. I dislike the use of system properties when they are not necessary. If
> > the class name was handled as a servlet context initialization parameters
> > then Tomcat already has the necessary plumbing for global, per host and per
> > web application configuration.
>      ~~~~~~~~~~~~~~~~~~ Supported passing the class name from context
> initialization parameters. Still keep the feature from System Property. 
>      In our case, System Property can be easily passed in from such kind of
> Daemon-Watcher, For example: JSW.

I haven't looked at the details here but it might be useful for you that system properties are automaticaly supported in most Tomcat config files, e.g. in server.xml and context.xml. What should work if you already support a className attribute is className="${my.el.interpreter}" (using a system property reference) and setting the system property via "-Dmy.el.interpreter=some.class.i.Use". No need for special code or a fixed system property name for this type of use case.

Regards,

Rainer

Thank you very much for your information! 
I'll try to follow up with this approach.

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #3)
> 
> > > 4. I dislike the use of system properties when they are not necessary. If
> > > the class name was handled as a servlet context initialization parameters
> > > then Tomcat already has the necessary plumbing for global, per host and per
> > > web application configuration.
> >      ~~~~~~~~~~~~~~~~~~ Supported passing the class name from context
> > initialization parameters. Still keep the feature from System Property. 
> >      In our case, System Property can be easily passed in from such kind of
> > Daemon-Watcher, For example: JSW.
> 
> I haven't looked at the details here but it might be useful for you that
> system properties are automaticaly supported in most Tomcat config files,
> e.g. in server.xml and context.xml. What should work if you already support
> a className attribute is className="${my.el.interpreter}" (using a system
> property reference) and setting the system property via
> "-Dmy.el.interpreter=some.class.i.Use". No need for special code or a fixed
> system property name for this type of use case.
> 
> Regards,
> 
> Rainer

Created attachment 29704 [details]
Test case for ELInterpreterFactory

Created attachment 29705 [details]
ELInterpreterFactory

All have been fixed by your suggestion. Please take a look. Thanks.

(In reply to comment #3)
> Comment on attachment 29685 [details]
> ELInterpreterFactory
> 
> In principle this looks like a good idea.
> 
> I have a couple of concerns with the patch as currently written:
> 1. No documentation.
> 2. No test cases.
> 3. The use of enum for the default instance is rather odd.
> 4. I dislike the use of system properties when they are not necessary. If
> the class name was handled as a servlet context initialization parameters
> then Tomcat already has the necessary plumbing for global, per host and per
> web application configuration.
> 5. Error messages need to use the standard i18n support.

Created attachment 29706 [details]
Interface of ELInterpreter

Added License information at the front of java file

Created attachment 29707 [details]
ELInterpreterFactory

Added APLV2.0 into ELInterpreterFactory.java

Created attachment 29708 [details]
Test case for ELInterpreterFactory

Added APL into TestELInterpreterFactory.java

Hi Mark,

The code has been refined. Test cases and document also have been provided. 

Could you please take a look ?

Patch applied to trunk and 7.0.x with a few tweaks here and there and the addition of some very basic documentation.

It will be included in 7.0.37 onwards.