On Fri, Jul 23, 2010 at 12:50 PM, Bruno Aranda <[email protected]> wrote: > No please, it is passing now!! Forgive my mistake of re-using a constant > with a line return in it!
:-) > > It was a possible bug with MyFaces, really, because the methods to add CDATA > in the comments where not relying on startCDATAs or checking nested CDATAs, > which can be a potential bug in my opinion. it does not hurt so show some love for a comment, that explains why; and also points to potential PrimeFaces problems. -Matthias > > Bruno > > On 23 July 2010 11:46, 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 :-) >> >> -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
