On Fri, Jul 23, 2010 at 12:46 PM, Matthias Wessendorf <[email protected]> wrote: > I get the build failure as well... > LEt's revert the commit and ask Primefaces guy(s) to fix it.... > > BTW It is not the first issues that users find when running the lib > with MyFaces... > > Honestly, I don't understand why he does not even bother to test > against MyFaces, > especially since he is a committer :-)
Don't get me wrong: It is totally fine to run the demos with Mojarra, but simply extremely lame to not test w/ MyFaces (at least it looks like there is no testing), especially since some users run that combination. -Matthias > > -Matthias > > On Fri, Jul 23, 2010 at 12:28 PM, Werner Punz <[email protected]> wrote: >> Hi I have to add my 2c to the entire issue. >> The main issue still is mainly at Primefaces and here is the reason, >> primefaces uses an xml response type protocol in the background but does not >> escape the cdata blocks like we and mojarra do in the ppr part. >> Instead it just uses the standard response writer to emit xml and expects a >> certain behavior type which is not clearly specified, to the worse I expect >> that it runs still into some problems because of missing fixup code on its >> side. >> >> Now I am doing some guessin ghere: >> >> Now if we suppress nested CDATA blocks there still will be usecases where >> primefaces will fail (Just a guess, Primefaces might have fixed it, so if I >> am wrong then beat me for it). The problem simply is if you use xml as >> response format with a suppressing behavior: >> >> >> imaging following valid html codeblock >> >> <script> >> //<![CDATA[ >> alert("<hello world>"); >> //]]> >> </script> >> >> or following >> <script> >> //<!-- >> alert("]]>"); >> //--> >> </script> >> >> Now what happens is following if we embed the first case and suppress the >> CDATA in our standard response writer >> >> <![CDATA[ >> <script> >> // >> alert("<hello world>"); >> // >> </script> >> ]> >> >> Which is very likely to fail in a parser in strict xhtml because >> after the ppr processing following is sent to the engine: >> <script> >> // >> alert("<hello world>"); >> // >> </script> >> >> The correct output would be here: >> <![CDATA[ >> <script> >> //<![CDATA[ >> alert("<hello world>"); >> //]]><![CDATA[]]]]><![CDATA[[> >> </script> >> ]]> >> >> >> The second case is even worse because it results on primefaces side im >> <![CDATA[ >> <script> >> //<!-- >> alert("]]>"); >> //--> >> </script> >> ]]> >> which is an absolute no go in xml because you can embed everything into >> CDATA blockes except ]]> >> >> unless Primefaces does not do its own escaping on the strings (which it very >> unlikely does not do. >> >> The correct output on this would look like: >> >> The second case is even worse because it results on primefaces side im >> <![CDATA[ >> <script> >> //<!-- >> alert("]]><![CDATA[]]]]><![CDATA[[>"); >> //--> >> </script> >> ]> >> >> But both correct responses are only correct if you expect xml which in >> itself embeds html code, so this is a no go for a standard response writer >> which is expected just to emit xhtml/html code with nothing embedded. >> >> So as I said the problem is not as easy at the first look and the ultimate >> fix must happen on the primefaces side here. >> >> The problem simply is if you use the html response writer for straight xml >> encoding with embedded html what is happening here, then you run into those >> problems, hence we have our own ppr response writer and also mojarra which >> exactly deals with those kinds of problems on ppr level. >> >> Dont get me wrong I dont mind having a fix for nested CDATA blocks in the >> html response writer, since the html response writer is for html this is >> correct, but that won´t resolve all problems primefaces has here, because it >> has to take into consideration of all this (and I have an eery feeling that >> the suppressing fix on the Myfaces side has to be disabled for the ppr case >> so that the ppr fixup code works properly because it delegates the html >> response writer and fixes the issue afterwards) >> >> >> >> >> Werner >> >> >> >> Am 22.07.10 20:11, schrieb Bruno Aranda: >>> >>> But we are talking about the HtmlResponseWriterImpl, which outputs HTML. >>> The fix I have done is in that class and it only checks if there is a >>> CDATA already started before starting one when writing the scripts. It >>> is slightly different to the original problem (about the HtmlResponse, >>> which is different from the one in Mojarra). The fix simply checks if >>> there is the CDATA around the script before opening another one inside >>> the script. I think it is OK if we just do not nest CDATAs in the HTML >>> response, even if it was allowed. >>> >>> And this fixes the problems with PrimeFaces too, without having to >>> change the ResponseWriter or the PartialResponseWriterImpl... >>> >>> Bruno >>> >>> On 22 July 2010 16:59, Werner Punz <[email protected] >>> <mailto:[email protected]>> wrote: >>> >>> Hia guys please also read up on my jira response. >>> The entire thing is not as easy as it seems, because there are >>> various ways a cdata block can be opened, first you can do it via >>> startCDATA secondly you can do it via a direct write. >>> >>> I did some kind of double buffering in case of a cdata block was >>> opened and then escaped the ]]> as multiple cdata blocks (the jira >>> response explains the technique exactly) >>> >>> And yes there is somewhat a speed hit because of this, but in case >>> of the partial response writer I did not have a chance because: >>> >>> But the partial response writer is somewhat different, because it >>> has to press html code in a valid xml response format, so nested >>> cdata blocks can occur naturally, the normal response writer is >>> somewhat different because nested cdata blocks are only forbidden >>> for xmlish output dialects others might allow it. >>> >>> Werner >>> >>> >>> >>> Am 22.07.10 17:47, schrieb Mark Struberg: >>> >>> >>> But isn't the patch of Marcus Büttner doing this by maintaining >>> a reference >>> counter? >>> >>> Another question: how is the performance of all this >>> scanning/dynamic >>> replacement? >>> >>> LieGrue, >>> strub >>> >>> >>> From: Bruno Aranda<[email protected] >>> <mailto:[email protected]>> >>> To: MyFaces Development<[email protected] >>> <mailto:[email protected]>> >>> Sent: Thu, July 22, 2010 5:26:35 PM >>> Subject: Re: Fixing ResponseWriter.startCDATA/endCDATA >>> >>> Further investigation of this incompatibility problem with >>> myfaces leads me to >>> the fact that in the HtmlResponseWriterImpl, when we write >>> the content of a >>> script, we create a CDATA element without checking if is >>> nested at all. That is >>> >>> >>> a problem, because if we use the standard response writer >>> and we write a script >>> >>> >>> section inside a CDATA section, the problem will be >>> triggered... >>> >>> We need a way in HtmlResponseWriterImpl to check nested >>> CDATA calls to the >>> startCDATA or endCDATA methods I guess. >>> >>> Cheers, >>> >>> Bruno >>> >>> >>> On 22 July 2010 15:15, Bruno Aranda<[email protected] >>> <mailto:[email protected]>> wrote: >>> >>> Just clicked on sent and Werner had answered in the JIRA >>> issue explaining the >>> partial approach... >>> >>> >>> Cheers, >>> >>> Bruno >>> >>> >>> >>> On 22 July 2010 15:12, Bruno >>> Aranda<[email protected] >>> <mailto:[email protected]>> wrote: >>> >>> As you can see in my black box tests with Mojarra, the >>> behaviour is different in >>> >>> both implementations. In the base ResponseWriter class, >>> they don't do anything >>> >>> >>> in the startCDATA method and throw an undocumented >>> exception in the endCDATA. >>> >>> >>> In both implementations of the base class, they >>> throw an exception if the >>> startCDATA method is called and it had been called >>> already... >>> >>> I don't quite understand our implementation of the >>> PartialResponseWriterImpl. We >>> >>> do buffer nested CDATAs and write them when closing >>> the parent one? This would >>> >>> >>> still create nested CDATAs... I still need to >>> understand this bit properly, >>> >>> Cheers, >>> >>> Bruno >>> >>> >>> >>> On 22 July 2010 13:58, Bruno >>> Aranda<[email protected] >>> <mailto:[email protected]>> wrote: >>> >>> yeah, sorry, my problem was running only the API >>> tests :) >>> >>> >>> Bruno >>> >>> >>> >>> On 22 July 2010 13:48, Matthias >>> Wessendorf<[email protected] >>> <mailto:[email protected]>> wrote: >>> >>> On Thu, Jul 22, 2010 at 2:14 PM, Matthias >>> Wessendorf<[email protected] >>> <mailto:[email protected]>> >>> >>> wrote: >>> >>> so, maybe there are now regressions? >>> >>> hrm. have you done some testing? >>> >>> >>> Ah, the discussion is on the JIRA.. >>> >>> please run tests, before committing ;-) >>> >>> >>> >>> -M >>> >>> On Thu, Jul 22, 2010 at 2:07 PM, >>> Matthias Wessendorf<[email protected] >>> <mailto:[email protected]>> >>> >>> wrote: >>> >>> sounds right. >>> >>> does blame say more why it does not >>> do nothing? >>> >>> It is also kinda strange since the >>> TCK was successfully executed for >>> 2.0.0 and 2.0.1; >>> >>> -Matthias >>> >>> On Thu, Jul 22, 2010 at 1:48 PM, >>> Bruno Aranda<[email protected] >>> <mailto:[email protected]>> >>> >>> wrote: >>> >>> Hi, >>> >>> Having problems with Primefaces >>> again I have realised that >>> something >>> >>> was >>> >>> working with Mojarra, but not >>> with MyFaces. Again, is the >>> ResponseWriter.startCDATA stuff >>> which Primefaces invokes directly >>> on >>> >>> its >>> >>> main phase listener. >>> >>> However, reading the javadocs: >>> >>> >>> https://javaserverfaces.dev.java.net/nonav/docs/2.0/javadocs/index.html >>> >>> It says that method "should >>> take no action when invoked"... >>> which >>> >>> means >>> >>> that it should be completely >>> empty as far as I understand. If >>> that was >>> >>> the >>> >>> case, we would get the same >>> behaviour in both >>> implementations... >>> >>> Cheers, >>> >>> Bruno >>> >>> >>> >>> >>> -- >>> Matthias Wessendorf >>> >>> blog: >>> >>> http://matthiaswessendorf.wordpress.com/ >>> sessions: >>> http://www.slideshare.net/mwessendorf >>> twitter: http://twitter.com/mwessendorf >>> >>> >>> >>> >>> -- >>> Matthias Wessendorf >>> >>> blog: >>> http://matthiaswessendorf.wordpress.com/ >>> sessions: >>> http://www.slideshare.net/mwessendorf >>> twitter: http://twitter.com/mwessendorf >>> >>> >>> >>> >>> -- >>> >>> Matthias Wessendorf >>> >>> blog: http://matthiaswessendorf.wordpress.com/ >>> sessions: http://www.slideshare.net/mwessendorf >>> twitter: http://twitter.com/mwessendorf >>> >>> >>> >>> >>> >>> >>> >>> >>> >>> >>> >>> >> >> >> > > > > -- > Matthias Wessendorf > > blog: http://matthiaswessendorf.wordpress.com/ > sessions: http://www.slideshare.net/mwessendorf > twitter: http://twitter.com/mwessendorf > -- Matthias Wessendorf blog: http://matthiaswessendorf.wordpress.com/ sessions: http://www.slideshare.net/mwessendorf twitter: http://twitter.com/mwessendorf
