[MAGNOLIA-1698] Review usage of originalURI/currentURI, forward/include, virtualUriFilter Created: 23/Aug/07  Updated: 23/Jan/13  Resolved: 01/Nov/07

Status: Closed
Project: Magnolia
Component/s: core
Affects Version/s: 3.5 RC1
Fix Version/s: 3.5 RC1

Type: Task Priority: Major
Reporter: Magnolia International Assignee: Fabrizio Giustina
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Attachments: Text File MAGNOLIA-1698.patch    
Issue Links:
dependency
depends upon MAGNOLIA-1776 Changing template renderers in order ... Closed
is depended upon by MAGNOLIA-1196 Using MagnoliaFilterChain as <dispatc... Closed
relation
is related to MAGNOLIA-1653 VirtualURI feature is broken in 3.1M1 Closed
is related to MAGNOLIA-560 Too eager caching in Path.getURI Closed
Template:
Acceptance criteria:
Empty
Task DoR:
Empty
Date of First Response:

 Description   

We should review

  • behaviour, usage and semantics of AggregationState's originalUri, currentUri
  • virtualUriFilter's behaviour (constants are wrongly named, behaviour unclear with regards to prefixes, etc)
  • JspTemplateRenderer's usage of RequestDispatcher.forward() - should probly use include()
  • the main filter definition in web.xml should maybe have
    <dispatcher>ERROR</dispatcher>
    <dispatcher>FORWARD</dispatcher>
    <dispatcher>REQUEST</dispatcher>


 Comments   
Comment by Magnolia International [ 24/Aug/07 ]

just for the record, here are a couple of change Philipp and I tried out - ! not be applied as is without further thinking and refining

Comment by Philipp Bracher [ 27/Aug/07 ]

Would like to explain that with an example to show why I consider that as very important topic.

Assume you implement a controller (using a magnolia page, servlet or other framework). Now after executing your controllers code you want to forward to a magnolia page (/forum/register-user.html for example)

This fails because the filter chain is not re-executed and the cms rendering won't work ( you end up in a 404).

If the configuration of the filer would be defined by using <dispatcher>FORWARD</dispatcher> the filter would be re-executed on a forward.

But after changing that we run into problems when we call a page directly (404). This happens because the JspTemplateRenderer uses a forward instead of an include what triggers a double execution of the filter also for single page calls.

To avoid that we change JspTemplateRenderer so that it uses an include (now a normal page is rendered nicely again).

Has anyone concerns about changing that (using include instead of a forward)?

If we change that we have to change the the virtual uri mapping filter as well, since it does some 'workarounds' to re-execute the filter chain itself.

The original uri is then set when the filter is executed (also when you do a forward) the current uri needs to be renamed to something like 'resolved uri' because this is the uri resolved by the virtual uri mappings. The resolved uri is used to read the right content node, the original uri is used to create the cache key.

I'd like to do that rather sooner than later since this facilitates the integration of other frameworks.

Since the magniolia filter chain uses bypasses you can avoid the re-execution by defining an other bypass, but you can not solve the described issue without having doing the changes we talked about.

So I vote for changing that immediately.

Comment by Philipp Bracher [ 02/Oct/07 ]

it's part of the 3.1 release because we have already done changes/cleanup in the filter execution and aggregation handling. This tasks seams to be very much related and increases the integration with other systems.

Comment by Fabrizio Giustina [ 03/Oct/07 ]

I agree that this should be changed asap... this could require some configuration change for people using magnolia together with a mvc framework, but it will also solve tons of problems.
Since it's anyway an important change I think that it should be applied as soon as possible in current milestones and not too late in the 3.1 release cycle. WDYT? Can we do that for 3.1-m4?

Comment by Philipp Bracher [ 05/Oct/07 ]

We should not forget to make the filters configurable in a way that one can define each filters behavior.

Actually we had some internal discussions about releasing 3.1 as soon as possible and doing a 3.2 release very soon afterward (february). We would then add only a few things like this issue to the 3.2. So that we solve only the critical issues now. But no final decision yet.

Comment by Fabrizio Giustina [ 27/Oct/07 ]

Not sure when we want to make this jump, but after the latest changes already in svn for 3.1 this looks easier now

3 steps:

  • modify ContentTypeFilter and add MgnlContext.resetAggregationState() at the beginning of doFilter()
  • modify VirtualUriFilter so that any "standard" magnolia virtual uri (the ones without any prefix) is handled like normal forwards (like the ones with "forward:" prefix)
  • add the dispatchers in WEB.xml

I have tested this for a couple of weeks and I was happily able to use forwards and error pages within magnolia, give it a try

Comment by Philipp Bracher [ 31/Oct/07 ]

I think you can commit. My arguments for a commiting now are:

  • tested locally
  • forward to include in template renderer is comited anyway
  • servlets already moved
  • we have to update web.xml anyway

Can you first add a conditional task which checks that the dispatching is added in web.xml

Can you also try to do a single commit, so that reverting is easy (we lack the time of chasing down things)

Comment by Fabrizio Giustina [ 01/Nov/07 ]

Committed to svn, now the magnolia filter works properly with forward and error dispatchers. A new update task check for the existence of the dispatcher element in web.xml

Generated at Mon Feb 12 03:29:27 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.