changcheng / wro4j

Automatically exported from code.google.com/p/wro4j
0 stars 0 forks source link

WroFilter#newWroConfigurationFactory() extendability is difficult #478

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
Althought newWroConfigurationFactory() is protected it is difficult to override 
it to specify custom PropertiesAndFilterConfigWroConfigurationFactory

return new PropertiesAndFilterConfigWroConfigurationFactory(filterConfig);

As last one requires filterConfig and we are unable to get it neither from init 
because it is final (any reason for this?)
public final void init(FilterConfig)
nor by
doInit(FilterConfig)
because it is called too late.

So had to use composition instead:

  private FilterConfig filterConfig;

  private WroFilter wroFilter;

  @Override
  public void init(final FilterConfig filterConfig) throws ServletException {
    this.filterConfig = filterConfig;

    wroFilter = new WroFilter() {
      @Override
      protected ObjectFactory<WroConfiguration> newWroConfigurationFactory() {
        return CustomWroFilter.this.newWroConfigurationFactory();
      }
    };

    wroFilter.init(filterConfig);
  }

  @Override
  public void destroy() {
    wroFilter.destroy();
  }

Is it intended use of API or can it be simplified?

Note in my newWroConfigurationFactory I load (override) additional 
wro-debug.properties (permanent by enabled by default if in development mode ) 
and wro-override.properties (just local file for develper to play with if 
needed not touching above two).

Original issue reported on code.google.com by lystoc...@gmail.com on 2 Jul 2012 at 11:47

GoogleCodeExporter commented 9 years ago
You don't have to provide a custom implementation of 
PropertiesAndFilterConfigWroConfigurationFactory in 
newWroConfigurationFactory() method. If you want to create a custom way of 
creating properties you have the following options:

1) Use setter: WroFilter.setConfiguration(config). Once the config is set, the 
ConfigurationFactory won't be used
2) Use newWroConfigurationFactory() and provide an implementation of 
ObjectFactory<WroConfiguration>. The 
PropertiesAndFilterConfigWroConfigurationFactory is used by default for 
backward compatibility reason (because earlier versions of wro4j were 
configured using init-param in web.xml). If you don't want to support that, you 
can use PropertyWroConfigurationFactory which is a simpler alternative to creat 
config from a properties file, or just build a brand new factory. 

Do you need FilterConfig for your custom factory? 
If the above solutions does not work for you, how would you like it to be?

Original comment by alex.obj...@gmail.com on 3 Jul 2012 at 7:20

GoogleCodeExporter commented 9 years ago
I don't use init-param but I do access servletContext from filterConfig the 
same as in WroFilter, so just being able to to override init would be ideal I 
think because this way I can initialize my own filterConfig in my init before 
calling super.init() reusing it's logic. 
The alternative for me could be to move properties to classpath instead of 
WEB-INF and maybe somehow use ConfigurableWroFilter with 
PropertyWroConfigurationFactory but that sounds more complicate then what I 
already done using composition and extending 
ServletContextPropertyWroConfigurationFactory to load more properties.

Original comment by lystoc...@gmail.com on 3 Jul 2012 at 10:43

GoogleCodeExporter commented 9 years ago
It would be good to be able to specify the properties you want to load only in 
one place. Currently I need to do in in both my 
CustomWroFilter#newWroConfigurationFactory() and my 
CustomConfigurableWroManagerFactory#newConfigProperties().

Original comment by lystoc...@gmail.com on 3 Jul 2012 at 10:48

GoogleCodeExporter commented 9 years ago
You cannot specify the properties in one place, simply because 
newWroConfigurationFactory() is not aware about ConfigurableWroManagerFactory. 
However you can avoid duplication by creating a PropertyFactory (a class 
responsible for creating the Properties object) which would be reused in both 
situations. 

As you can see, there is a lot of options which is up to your use-case.. The 
default implementations are not necessarily the best. If you think there is a 
better approach, I'm open to suggestions. 

I would like to define some actions for this task, otherwise it will remain 
unchanged, since it is not really clear what exactly has to be done.

Original comment by alex.obj...@gmail.com on 3 Jul 2012 at 11:04

GoogleCodeExporter commented 9 years ago
Task is to open init method for extension or explain why it is closed for 
extension.

Original comment by lystoc...@gmail.com on 4 Jul 2012 at 12:07

GoogleCodeExporter commented 9 years ago
Is it ok if the newWroConfigurationFactory() method will take FilterConfig as 
parameter? 

The init method is closed for extension because it performs critical 
initialization which shouldn't be overridden by client code. That is why 
doInit() method exist.

Original comment by alex.obj...@gmail.com on 5 Jul 2012 at 7:20

GoogleCodeExporter commented 9 years ago

Original comment by alex.obj...@gmail.com on 5 Jul 2012 at 7:20

GoogleCodeExporter commented 9 years ago
> Is it ok if the newWroConfigurationFactory() method will take FilterConfig as 
parameter?

Yes it is also acceptable in my case.

Original comment by o...@omax.org.ua on 5 Jul 2012 at 4:29

GoogleCodeExporter commented 9 years ago
Fixed in branch 1.4.x.

Original comment by alex.obj...@gmail.com on 8 Jul 2012 at 10:10