Re: Please review WICKET-983: Merge the portlet support branch into the trunk

[Al Maw <wi...@almaw.com>]

Ate Douma wrote:
> I do understand the concerns raised but I'd like to point out that, at 
> least in my view, the impact for *normal* wicket runtime (e.g. servlet, 
> not portlet) the changes really are very, very minimal.
> 
> Al, Gwyn and Kent: you indicate that you see major and/or fundamental 
> changes to wicket, but please make explicit which changes you are 
> referring to.
> Now I only have something like "the portlet changes /do/ touch some 
> fundamental bits of Wicket",  "making major changes to the codebase" and 
> "It seems that those changes are quite fundamental" to respond to.
> 
> Anyway, I'll try to make clear why in my view the portlet changes aren't 
> going to cause "major" or "fundamental" changes in the core wicket.

They're not major changes, but they do touch some fundamental bits of 
Wicket. :-)

I'm specifically thinking about the URL handling, which isn't terribly 
well unit tested and has in the past proved to be rather brittle. I'd be 
quite surprised if you haven't introduced something subtle here, as 
there are so many facets (interaction with the various 
UrlCodingStrategies (both with and without parameters), getting it right 
for both WicketServlet and WicketFilter, auto-prepending relative paths 
to raw src/href/whatever attributes in HTML, getting it right when 
outputting AJAX fragments for requests made to a URL that's not at the 
same relative depth as the URL for the page it's from, etc. etc.).

> If you evaluate the issues I listed in WICKET-983 and how I resolved 
> those, you'll see that the changes which *might* have a small to zero 
> effect on the standard wicket behavior are the following:
> [...]

Thanks for your comprehensive list. It's very helpful. I'll need to 
spend some time evaluating these in some detail in order to say whether 
I'm happy with them or not, but won't really be able to look before 
about Sunday. :-(

Here are my comments on the things that I don't need to look at the code 
for...

> 4) WICKET-657: upgrading wicket-examples to require servlet api 2.4

I have no objection to that one.

> 5) WICKET-924: non-relative urls in Ajax.Request.redirect callback handling
> Actually, I would consider proposing this change regardless of the 
> discussion of portlet-support. The current hard coded expectancy an Ajax 
> callback redirect
> can and may only be a Wicket relative url doesn't seem very flexible. 
> For the portlet-support this change is needed though.
> And, if the redirect url *is* relative as expected in the current 
> implementation, my changes won't do anything anyway.
> But if there are major objections to it, we could discuss solving it in 
> a similar way as we could do with the Ajax.Request.get(path) -> 
> .post(body) change.
> If we can push the current runtime environment (servlet|portlet) to the 
> browser the ajax script could maintain the current behavior when running 
> in a servlet.

The issue here is that we've made all the URLs relative for a good 
reason - to let you run Wicket behind a mod_proxy without having lots of 
pain. Additionally, you need the URLs in generated AJAX fragments to be 
relative to the page that the request was made from, not relative to the 
request itself. Your patch needs to make sure this behaves properly.

> Delaying portlet-support to after the 1.3 release really would be a 
> shame in this light in my view.

Yes, I agree. It's really hard to maintain a patchset of this size, and 
squeezing it out to 1.4 (or whatever we're calling the next version) 
would make for unnecessary work if we can get it into 1.3 with a 
guarantee that it's not going to break things.

I also suspect that we have a bunch of 1.2.x portlet users who we're 
going to leave high-and-dry otherwise, which is certainly a consideration.

> If anyone of you still have reservations and/or major objections of 
> merging portlet-support in the current trunk, please give it head and 
> toe and indicate which changes you think are questionable so we can 
> properly discuss them.

I'll have a proper in-depth review of your patch later in the week and 
let you know.

Regards,

Al

Re: Please review WICKET-983: Merge the portlet support branch into the trunk

[Ate Douma <at...@douma.nu>]

Al, thanks for the reply.

I have a few comments inline below.

Al Maw wrote:
> Ate Douma wrote:
>> I do understand the concerns raised but I'd like to point out that, at 
>> least in my view, the impact for *normal* wicket runtime (e.g. 
>> servlet, not portlet) the changes really are very, very minimal.
>>
>> Al, Gwyn and Kent: you indicate that you see major and/or fundamental 
>> changes to wicket, but please make explicit which changes you are 
>> referring to.
>> Now I only have something like "the portlet changes /do/ touch some 
>> fundamental bits of Wicket",  "making major changes to the codebase" 
>> and "It seems that those changes are quite fundamental" to respond to.
>>
>> Anyway, I'll try to make clear why in my view the portlet changes 
>> aren't going to cause "major" or "fundamental" changes in the core 
>> wicket.
> 
> They're not major changes, but they do touch some fundamental bits of 
> Wicket. :-)
> 
> I'm specifically thinking about the URL handling, which isn't terribly 
> well unit tested and has in the past proved to be rather brittle. I'd be 
> quite surprised if you haven't introduced something subtle here, as 
> there are so many facets (interaction with the various 
> UrlCodingStrategies (both with and without parameters), getting it right 
> for both WicketServlet and WicketFilter, auto-prepending relative paths 
> to raw src/href/whatever attributes in HTML, getting it right when 
> outputting AJAX fragments for requests made to a URL that's not at the 
> same relative depth as the URL for the page it's from, etc. etc.).
I know the URL handling is quite complex :)
It was quite difficult to find the right spots where to intercept/provide alternate behavior for portlets.
And I agree, I might (and probably haven't) covered all the specific issues yet... for portlets.
The point I wanted to make is, for servlets behavior *hasn't* changed, except for the few specific areas I found where query string parameters are appended 
manually (and I might have missed a few still).
So, while the url handling for portlets might not be perfect yet, this will only cause problems for portlets.
I do think do I covered the most common cases already enough to get started with real portlet development with Wicket.
Bugs will always be there, and in the case of portlets, I'll take the responsibility to maintain and fix those as best as I can.
But for servlets, this really won't introduce new issues beyond what is already brittle or even broken in the first place.

> 
>> If you evaluate the issues I listed in WICKET-983 and how I resolved 
>> those, you'll see that the changes which *might* have a small to zero 
>> effect on the standard wicket behavior are the following:
>> [...]
> 
> Thanks for your comprehensive list. It's very helpful. I'll need to 
> spend some time evaluating these in some detail in order to say whether 
> I'm happy with them or not, but won't really be able to look before 
> about Sunday. :-(
That's ok for me, I just hope the others can wait with the -beta4 cutoff that long too.
> 
> Here are my comments on the things that I don't need to look at the code 
> for...
> 
>> 4) WICKET-657: upgrading wicket-examples to require servlet api 2.4
> 
> I have no objection to that one.
> 
>> 5) WICKET-924: non-relative urls in Ajax.Request.redirect callback 
>> handling
>> Actually, I would consider proposing this change regardless of the 
>> discussion of portlet-support. The current hard coded expectancy an 
>> Ajax callback redirect
>> can and may only be a Wicket relative url doesn't seem very flexible. 
>> For the portlet-support this change is needed though.
>> And, if the redirect url *is* relative as expected in the current 
>> implementation, my changes won't do anything anyway.
>> But if there are major objections to it, we could discuss solving it 
>> in a similar way as we could do with the Ajax.Request.get(path) -> 
>> .post(body) change.
>> If we can push the current runtime environment (servlet|portlet) to 
>> the browser the ajax script could maintain the current behavior when 
>> running in a servlet.
> 
> The issue here is that we've made all the URLs relative for a good 
> reason - to let you run Wicket behind a mod_proxy without having lots of 
> pain. Additionally, you need the URLs in generated AJAX fragments to be 
> relative to the page that the request was made from, not relative to the 
> request itself. Your patch needs to make sure this behaves properly.
Not really...
Understand that a Wicket portlet runs *embedded* in a page and doesn't control the (browser) url anyway.
For portals, this is a known issue with any portlet url, regardless which framework the portlet is developed in.
For the portlet-support I actually had to rewrite the Wicket relative urls back to context relative urls to be able to serve them from a portlet!
But my change above does not affect plain (relative) Wicket urls at all, it just caters for the cases where the url happens to be fully qualified anyway.

> 
>> Delaying portlet-support to after the 1.3 release really would be a 
>> shame in this light in my view.
> 
> Yes, I agree. It's really hard to maintain a patchset of this size, and 
> squeezing it out to 1.4 (or whatever we're calling the next version) 
> would make for unnecessary work if we can get it into 1.3 with a 
> guarantee that it's not going to break things.
> 
> I also suspect that we have a bunch of 1.2.x portlet users who we're 
> going to leave high-and-dry otherwise, which is certainly a consideration.
> 
>> If anyone of you still have reservations and/or major objections of 
>> merging portlet-support in the current trunk, please give it head and 
>> toe and indicate which changes you think are questionable so we can 
>> properly discuss them.
> 
> I'll have a proper in-depth review of your patch later in the week and 
> let you know.
> 
> Regards,
> 
> Al
> 
Thanks a lot already!

Regards,

Ate