Skip to content
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

Struts 2.3.x: Tiles plugin upgrade #60

Merged
merged 15 commits into from Jan 10, 2016
Merged

Struts 2.3.x: Tiles plugin upgrade #60

merged 15 commits into from Jan 10, 2016

Conversation

lukaszlenart
Copy link
Member

Fixes WW-4568

This PR upgrades the Tiles Plugin and adjusts its API to be compatible with Tiles 2.2.2 - this allows introduce support for OGNL, EL, Wildcards and so on.

(cherry picked from commit 85b2198)
(cherry picked from commit 24a5bce)
(cherry picked from commit 26f153d)
(cherry picked from commit 6edc0ff)
(cherry picked from commit 27ad6cb)
(cherry picked from commit 388861f)
(cherry picked from commit 6622fab)

Conflicts:
	apps/showcase/pom.xml
	plugins/bean-validation/pom.xml
@lukaszlenart lukaszlenart mentioned this pull request Nov 28, 2015
@cnenning
Copy link
Member

For struts 2.3 I still got an old app with tiles2 which does some custom stuff and which has issues with these changes.

  • tiles-servlet-wildcard depends on spring-web. That causes spring to be pulled in to all apps that use struts-tiles-pluign. Due to this I would prefer wildcard support could be optional
  • in TilesContainerFactory.createContainerFactory() it is necessary for me to register tilesContainer in servletContext. Can be done like this, would you mind adding this to StrutsTilesContainerFactory?
@Override
protected BasicTilesContainer instantiateContainer(TilesApplicationContext applicationContext) {
    CachingTilesContainer tilesContainer = new CachingTilesContainer();
    ServletContext servletContext = (ServletContext) applicationContext.getContext();
    ServletUtil.setContainer(servletContext, tilesContainer);
    return tilesContainer;
}
  • in StrutsTilesContainerFactory.getSourceURLs() these two resource patterns are used: /WEB-INF/**/tiles*.xml and classpath*:META-INF/**/tiles*.xml. The 2nd one causes an exception in WebSphere with IBM JDK. Would be nice if we could make this configurable. The exception is Caused by: java.net.MalformedURLException: SRVE0238E: Resource paths should have a leading slash

Currently I have created my own TilesListener, TilesInitializer and TilesContainerFactory. With the above changes I could avoid that and just stick to StrutsTilesListener.

Two minor things I noticed in showcase app:

  • It logs this message: WARN (freemarker.jsp:75) - Custom EL functions won't be loaded because no ObjectWarpper was specified . So it may be that tiles-el along with tiles-freemarker are not working?
  • css layout seems to be broken in firefox (surely not related to this PR, looks good in chrome and ie11, is fixed in master branch)

@lukaszlenart
Copy link
Member Author

@cnenning thanks for a really good feedback. I implemented a dedicated ApplicationContext to support wildcard matching without depending on Spring - by default all tiles*.xml will be loaded.

After cleaning up dependencies I didn't notice the issue with EL, could you check that on your side?

Thanks!

@cnenning
Copy link
Member

The new version looks awesome!

Just a small addon to constructor of StrutsWildcardServletTilesApplicationContext, that re-enables loading of tiles*.xml from classpath and works in IBM stuff, too:

        Enumeration<URL> cpResources = getClass().getClassLoader().getResources("/");
        while (cpResources.hasMoreElements()) {
            URL cpResource = cpResources.nextElement();
            urls.add(cpResource);
        }

Regarding the other issues:

  • css looks alright again in FF, seems like it was a FF bug
  • About EL stuff: I'm pretty helpless and pretty confused. That log message is created in freemarker.ext.jsp.TaglibFactory. It looks for a BeansWrapper which struts creates in FreemarkerManager.createObjectWrapper(). When tiles-layout is created with a JSP, and just tiles-attributes are done with FTLs everything seems OK. But when tiles-layout is done with FTL that message is logged and EL support seems broken.

@lukaszlenart
Copy link
Member Author

I still cannot reproduce the issue with missing support for EL, here is my example I have been using to test this PR

https://github.com/lukaszlenart/struts2-tiles-demo

@cnenning
Copy link
Member

cnenning commented Jan 5, 2016

https://github.com/lukaszlenart/struts2-tiles-demo

I created a PR for that demo app which demonstrates the issue.

See https://github.com/lukaszlenart/struts2-tiles-demo/pull/2

@lukaszlenart
Copy link
Member Author

This is what I get (I have been using Jetty)
2016-01-06_0825

@cnenning
Copy link
Member

cnenning commented Jan 7, 2016

This is what I get (I have been using Jetty)

Hmm, the freemarker template is not executed but it's code is shown. This is another issue. No idea why that happens.

@cnenning
Copy link
Member

cnenning commented Jan 7, 2016

I would say we merge this and may fix the "EL not working in freemarker-insert with freemarker-template" issue in master.

@lukaszlenart
Copy link
Member Author

Let's do it, at least I will have less problem with debugging :D

@asfgit asfgit merged commit 5ed7b36 into apache:support-2-3 Jan 10, 2016
@lukaszlenart lukaszlenart deleted the tiles-plugin-upgrade-struts-2-3 branch January 10, 2016 11:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants