Re: [xwiki-devs] [Proposal] Adding new velocity methods to simulate try/catch in Velocity

Hi, Interesting, though I do have my worries about having to deal with actual exceptions in velocity. Probably the most common usecase would be just to display the exception, but others may now be tempted to look at exception types and display custom messages for each error situation. That should improve the quality of the UIs, but at the same time I feel like it will increase the amount of velocity code in wiki pages. Generally, I`m +1 since I had to deal on some occasions with exception handling in velocity and found myself re-coding over and over again the methanism of passing the exception from the script service to the velocity UI. However, I feel there are some things that are could be done to improve it in practice: 1. Make sure that the "try" directive clears any existing context exception before executing it's content and possibly setting a new exception in the context. Example: #try() #end #try() #end

2. Support nested exception handling. Example: #try() #try() #end #end I agree that this point (2.) could probably be solved by (1.) if the "try" directive clears the context exception *after* executing the code so that any leftovers from nested exceptions are cleared. 2.1. However, my initial suggestion was to maybe support "named exceptions" (i.e. a parameter for the "try" directive that could specify the variable where to expose the exception). Example: #try() #try("nestedException") <-- or #try($nestedException) but I think this is more problematic since it really has to be set to $NULL before #end #end 2.2 Thinking of the above and the nested usecase, I just wanted to mention a possible #throw($exception) directive in case we want to use the main "try" block as a catch-all handler, even if nested try blocks caught and reported errors as they occurred (so they would now bubble the exception towards the main try block to signal that the operation as a whole was problematic). Example: #try() #try() #end #if ($exception) #throw($exception) <-- this probably stops the execution here. #end #end Of course, this can be done with a variable instead in the exception handling of each nested try blocks. Example: #try() #try() #end #if ($exception) #set($failed=true) <-- this leaves the developer to decide by using some IFs. #end #end Point 1. is a must IMO to avoid unwanted errors. The rest is just food for thought, perhaps you consider them useful as well.

Thanks, Eduard On Fri, Sep 19, 2014 at 3:48 PM, vincent(a)massol.net wrote: > > > > > On 19 Sep 2014 at 10:24:51, Thomas Mortagne (thomas.mortagne(a)xwiki.com > (mailto:thomas.mortagne@xwiki.com)) wrote: > > > #try > > > > was not possible ? (without parenthesis) > > AFAIK it’s not possible since BLOCK directives in Velocity need > parenthesis. > > Thanks > -Vincent > > > On Fri, Sep 19, 2014 at 12:04 AM, vincent(a)massol.net wrote: > > > > > > > > > On 18 Sep 2014 at 19:41:56, vincent(a)massol.net (vincent(a)massol.net > (mailto:vincent@massol.net)) wrote: > > > > > >> Hi Sergiu, > > >> > > >> On 18 Sep 2014 at 19:29:52, Sergiu Dumitriu (sergiu(a)xwiki.org(mailto: > sergiu(a)xwiki.org)) wrote: > > >> > > >> > 1. I would rather add proper directives instead of macros and > > >> > uberspector hacks: http://stackoverflow.com/q/159292/620249 > > >> > > >> Argh… I wanted that and I asked Thomas and he told me it wasn’t > possible and I didn’t double check. I should have since I’ve now finished > my implementation and redoing it as a directive means dumping what I’ve > done a third time in the end. Anyway I agree that a directive is better so > I’ll redo it. > > > > > > I’ve now implemented a directive and it’s so cool :) > > > > > > It’s so simple compared to what I had, I love it. > > > > > > Here’s the usage code now: > > > > > > #try() > > > #set($outputSyntax = > $xwiki.getAvailableRendererSyntax($request.outputSyntax, > $request.outputSyntaxVersion)) > > > #if ($outputSyntax) > > > ## If the passed syntax is not an HTML-compatible syntax we need to > HTML-escape it so that it can be > > > ## displayed fine in HTML (since at the point this vm file is called > we're already inside an HTML page with > > > ## panels on the side, header, etc). > > > #set($syntaxType = $outputSyntax.type.toIdString()) > > > #if (($syntaxType == "xhtml") || ($syntaxType == "html")) > > > #set ($renderedContent = $tdoc.getRenderedContent($outputSyntax)) > > > #else > > > ## Make sure to print correctly the result when it's not HTML > > > #set ($renderedContent = " > $escapetool.html($tdoc.getRenderedContent($outputSyntax)) > ") > > > #end > > > #else > > > #set ($renderedContent = $tdoc.getRenderedContent()) > > > #end > > > #end > > > … > > > > > > > #if ("$!exception" != '') > > > #displayException($exception) > > > #else > > > $renderedContent > > > #end > > > > > > > … > > > > > > I like it so much that I’m going to commit it and if someone doesn’t > agree I’ll revert the commit :) > > > > > > Thanks Sergiu > > > -Vincent > > > > > >> > 2. So far it has been the job of our scriptable wrappers to catch > > >> > exceptions, does this proposal mean that: > > >> > a. We should stop doing that and write try-catch logic in our > > >> > templates/scripts? IOW, catching exceptions in wrappers is a bad > design > > >> > pattern? > > >> > > >> I’ve not answered this question because I don’t know yet. I think > we’ll discover that as we start using the new try/catch. ATM I’m only > planning to use it in contentview.vm. > > >> > > >> > b. We didn't do that enough, and as a workaround we're adding > exception > > >> > handling in Velocity? > > >> > > >> Yes, that’s true. We now have both cases in our code (code throwing > exceptions, like a lot of methods in Document, XWiki) and others catching > exceptions. > > >> > > >> This proposals offers a finer-grained control in vm while not > breaking backward compatibility. Any change in our script APIs would break > compatibility (this is what I started doing before realizing it would break > too many things)… > > >> > > >> FTR here’s what I have so far in term of usage, in contentview.vm: > > >> > > >> #try() > > >> #set($outputSyntax = > $xwiki.getAvailableRendererSyntax($request.outputSyntax, > $request.outputSyntaxVersion)) > > >> #if ($outputSyntax) > > >> ## If the passed syntax is not an HTML-compatible syntax we need to > HTML-escape it so that it can be > > >> ## displayed fine in HTML (since at the point this vm file is called > we're already inside an HTML page with > > >> ## panels on the side, header, etc). > > >> #set($syntaxType = $outputSyntax.type.toIdString()) > > >> #if (($syntaxType == "xhtml") || ($syntaxType == "html")) > > >> #set ($renderedContent = $tdoc.getRenderedContent($outputSyntax)) > > >> #else > > >> ## Make sure to print correctly the result when it's not HTML > > >> #set ($renderedContent = " > > > $escapetool.html($tdoc.getRenderedContent($outputSyntax)) > > > ") > > >> #end > > >> #else > > >> #set ($renderedContent = $tdoc.getRenderedContent()) > > >> #end > > >> #catch() > > >> > > >> … > > >> > > > > > >> #if ("$!exception" != '') > > >> #displayException($exception) > > >> #else > > >> $renderedContent > > >> #end > > >> > > > > > >> … > > >> > > >> > > >> Thanks > > >> -Vincent > > >> > > >> > On 09/18/2014 09:28 AM, vincent(a)massol.net wrote: > > >> > > Hi Devs, > > >> > > > > >> > > Example of a use case: > > >> > > > > >> > > * In contentview.vm we need to be able to catch when > getRenderedContent() throws an exception in order to be able to still > display the page title, content menu and last modification line. We want > only the content part to display an error. > > >> > > > > >> > > I’ve tried implementing the following: > > >> > > * Modified Document.getRenderedContent() to not throw exceptions > any more > > >> > > * I’ve added Document.setError() and getError() > > >> > > * Then added a #displayRenderedContent($renderedContent) > velocimacro to display an error, calling Document.getError() > > >> > > > > >> > > However that’s no good because we have lots of places in our code > (and in extensions we don’t control) that call getRenderedContent() and > they expect the whole page rendering to fail in case of an error… > > >> > > > > >> > > Thus after discussing quickly on IRC and with Thomas we’d like to > propose instead: > > >> > > > > >> > > * Add a new #try() velocimacro that would push a new empty List > in the Execution Context to a Stack variable > > >> > > * Add a new uberspector that would, based on this flag do a > try/catch around the method called (can be done easily by returning a > custom VelMethod) > > >> > > * The uberspector catch would add the exception to the List in > the Execution Context > > >> > > * Add a new #catch() velocimacro that would pop from the Stack in > the Execution Context and that would set 2 variables in the Velocity > Context: > > >> > > ** $exception: the first exception > > >> > > ** $exceptions: the list of all exceptions that happened between > the #try() and #catch() > > >> > > > > >> > > The good part with this is: > > >> > > * No change in behavior to all existing code > > >> > > * Code that want to do something in case of error can do it using > #try()/#catch() for java methods throwing exceptions (like > getRenderedContent()) > > >> > > > > >> > > Of course for the future we would need to decide if script > services should stop doing try/catch or not but that’s another debate I’d > prefer to separate from this thread. > > >> > > > > >> > > WDYT? > > >> > > > > >> > > Thanks > > >> > > -Vincent