15 Jan
2016
15 Jan
'16
5:52 p.m.
What about making possible to enable/disable as a {{velocity}} macro
parameter (disabled by default) ?
On Fri, Jan 15, 2016 at 5:41 PM, vincent(a)massol.net <vincent(a)massol.net> wrote:
I need to think more about this because it means
velocity macros won’t show an error box anymore in case of error….
Thanks
-Vincent
On 15 Jan 2016 at 17:20:21, vincent(a)massol.net
(vincent@massol.net(mailto:vincent@massol.net)) wrote:
--
Thomas Mortagne
Ok I’ve finished the implementation and will be
committing soon.
Note that this could change slightly what the user sees.
For example in the past if you had this velocity script:
{{velocity}}
$someclass.someMethodThrowingException()
… rest of the script here...
{{/velocity}}
The user would see a stack trace on his screen before upgrading to 8.0 and after
upgrading to 8.0 he’ll see something different which depends on what the “… rest of the
script here…” does.
What I don’t like too much is that this could lead to unwanted side effects. For example,
imagine that we have the following:
{{velocity}}
#set ($docReference = $someclass.someMethodComputingTheReferenceButThrowingException())
…
$xwiki.getDocument($docReference).save()
...
{{/velocity}}
Then if an exception is raised in someMethodComputingTheReferenceButThrowingException()
it used to display an exception but will now do something else. It happens that in this
case it’ll report a NPE (AFAICS in the code) but we could imagine it would create a wrong
document in the wiki, etc.
Any thoughts about this? Should I still go ahead (I think so).
Thanks
-Vincent
On 15 Jan 2016 at 13:37:39, vincent(a)massol.net
(vincent@massol.net(mailto:vincent@massol.net)) wrote:
_______________________________________________
devs mailing list
devs(a)xwiki.org
http://lists.xwiki.org/mailman/listinfo/devs
On 15 Jan 2016 at 13:26:08, vincent(a)massol.net
(vincent@massol.net(mailto:vincent@massol.net)) wrote:
On 15 Jan 2016 at 13:23:07, vincent(a)massol.net
(vincent@massol.net(mailto:vincent@massol.net)) wrote:
> Forget what I’ve said regarding the #try directive.
>
> Actually the way to use it is:
>
> #try()
> … wrap some script snipper, one or several calls…
> #end
> .. here only you can access the $exception binding.
>
> Thus:
>
> * Wrapping the whole content to evaluate won’t work
> * The Exception Catching Uberspector is really what we need. It’s more fine-grained
than the #try directive (or it’s equivalent if you use #try() for each line of script
calling methods).
>
> In addition:
>
> * We provide script APIs in xwiki-commons too so if we want this best practice of
throwing exceptions in script APIs, we also need to make this work in xwiki-commons when
used outside of XWiki. Thus we need a solution there too.
> ** A) One solution would be to use XWiki’s Context and make the last exception
available with $xcontext.lastVelocityException but it’s not very natural, I hope we can
find a better solution
> ** B) Another solution is to move VelocityManager to commons in
xwiki-commons-velocity, offer a basic impl without skin support and override it in
xwiki-platform to add the platform-specific stuff.
>
> I’ll start exploring B). Let me know if you don’t agree or if you have a better
idea.
Actually VelocityManager is already in commons, it’s the impl that is in platform. So
right now it will work for us; it just won’t work for users who use just commons. Like if
you use the Rendering in standalone mode.
Actually forget that, I don’t need to use VelocityManager in practice (and it would be
costly to do so). I just need to use the Execution component:
VelocityContext vcontext =
(VelocityContext) this.execution.getContext().getProperty(
VelocityExecutionContextInitializer.VELOCITY_CONTEXT_ID);
Thanks
-Vincent
Ps: Sorry for the live design noise...
>
> Thanks
> -Vincent
>
> > Thanks
> > -Vincent
> >
> >
> > On 15 Jan 2016 at 12:15:41, vincent(a)massol.net
(vincent@massol.net(mailto:vincent@massol.net)) wrote:
> >
> > >
> > > [snip]
> > >
> > > > > > > > >> > Hi devs,
> > > > > > > > >> >
> > > > > > > > >> > Right now our strategy is for
script services and script APIs in general
> > > > > > > > >> > to catch exceptions, store them and
offer a getLastError() method to get
> > > > > > > > >> > them (see
> > > > > > > > >> >
http://extensions.xwiki.org/xwiki/bin/view/Extension/Script+Module#HBestPra…
> > > > > > > > >> > )
> > > > > > > > >> >
> > > > > > > > >> > However it would be much nicer to:
> > > > > > > > >> > * Let our script services generate
exceptions
> > > > > > > > >> > * Offer a velocity script service
to get the last exception raised by a
> > > > > > > > >> > java call from velocity
> > > > > > > > >> > * Implement this uberspector to
catch the exceptions and to set them in
> > > > > > > > >> > the execution context
> > > > > > > > >> >
> > > > > > > > >> > That should be quite easy to
implement IMO.
> > > > > > > > >> >
> > > > > > > > >> > WDYT?
> > > > > > > > >> >
> > > > > > > > >>
> > > > > > > > >> +1, it's a pain to call
setLastError() everywhere there can be an exception
> > > > > > > > >> thrown, and we almost always forget to
do it (for this reason).
> > > > > > > > >>
> > > > > > > > >> Note that we also have the #try()
directive now.
> > > > > > > > >
> > > > > > > > > Yes, I should have mentioned that there’s
indeed also this possibility:
> > > > > > > > > * Have script API throw Exceptions
> > > > > > > > > * Force velocity script users to wrap their
code with the try directive when they need to catch exceptions
> > > > > > > > >
> > > > > > > > > I still believe that the use of the
Exception-catching uberspector is better.
> > > > > > > > >
> > > > > > > > > WDYT?
> > > > > > > >
> > > > > > > > Does it mean you plan to get rid of new #try
directive ? Because it
> > > > > > > > will be broken with this new uberspector.
> > > > > > >
> > > > > > > That’s a good point, I had not thought about the
implementation at this stage.
> > > > > > >
> > > > > > > I think this could still work. When the #try directive
is used I’d just have to setup some flag somewhere in Velocity and in the uberspector I
could check if this flag is set and if so then don’t catch the exception.
> > > > > >
> > > > > >
> > > > > > Actually, thinking more, I think you’re right and that the
#try directive plays exactly the same role as an Exception-catching uberspector and I
don’t see the need for the #try directive if we provide an uberspector.
> > > > > >
> > > > > > So I’m proposing to deprecate it but still keep it for
backward compatibility for now (probably a full cycle).
> > > > > >
> > > > > > WDYT?
> > > > >
> > > > > Note that I’d like to change a bit the proposal and instead of
making the exception available from a script service, I’d prefer to make it available as a
known velocity binding such as $lastException. There’s no reason to use a script service
since that would mean it would work for all scripts and in this case we only want it to
work for Velocity.
> > > > >
> > > > > Since there’s no way to get the Velocity Context from within an
uberspector, I’ll get it by using our Component Manager and get the VelocityManager
component and call getVelocityContext()… If you know a better way, let me know.
> > > >
> > > > hmm… this would mean that I’d need to put this new uberspector in
xwiki-platform since VelocityManager is in platform ATM… (@Thomas: our discussion of
yesterday ;)).
> > >
> > >
> > > There’s an alternative, which is to modify our implementation of
VelocityEngine.evaluate() and decorate the source with a #try() directive so that it’s
always called (and make sure that calling it nested won’t affect it for backward compat).
> > >
> > > This could be simpler to implement and doesn’t force us to move some
velocity code to platform.
> > >
> > > WDYT?
> > >
> > > Thanks
> > > -Vincent
> > >
> > > [snip]
>
> _______________________________________________
> devs mailing list
> devs(a)xwiki.org
> http://lists.xwiki.org/mailman/listinfo/devs