Oh yeah, i understand that from your side totally! Paying of tech debt for big upgrades takes a long time, and i'm super grateful for your thoroughness this time around. I'm just saying from the Velocity dev side, it seems odd to re-add a large, complicated feature (the explicit macro scopes are much cleaner inside and out, IMO) after two versions without it, purely for a backward compatibility that was deprecated in a release 9+ years ago and hasn't been in a "latest and greatest" release for 3 years now. It's hard to justify the effort on our end. And a bit moot, since i haven't the time for it anyway. But again, if you or Claude or someone else wants to take it on yourselves, i won't veto at all. My favorite Apache Way maxim has always been that those who do the work should make the decisions!
On Thu, Jan 16, 2020 at 5:12 AM Thomas Mortagne <thomas.morta...@xwiki.com> wrote: > On Wed, Jan 15, 2020 at 5:31 PM Nathan Bubna <nbu...@gmail.com> wrote: > > > > I don't have any time for dev work on Velocity these days, so it doesn't > > much matter whether i consider it or not. :) If someone wants to put in > the > > work, i won't protest. > > > My inclination, ineffectual though it may be, is to > > point out that it was removed 2 versions ago, in 2.0. Adding it back in > as > > a BC option in 2.2 seems more than a little belated... :) > > I understand but I started working on Velocity 2.x upgrade in XWiki > quite a while ago and it took two versions because it was not very > straightforward with XWiki using quite a lot of the Velocity APIs and > many of those having changed a lot (some of which I'm very happy with > but was still some work) and then start testing actual use cases to > find out regressions or planned but not obvious breaking changes in > actual language from XWiki extensions point of view. My bad judgment > was to wait for 2.1 before testing more use cases because there was a > lot of retro compatible flags and bug fixes in it that I needed while > I should have tested right away with 2.1 SNAPSHOTS like I do now with > 2.2 but it was also not so easy to spot everything at once when those > were hidden by now fixed regressions or new retro compatibility flags. > > > > > I do agree, the 1.7-2.0 upgrade docs probably should have said that those > > deprecated features were actually removed. That was an oversight on our > > part. > > > > On Wed, Jan 15, 2020 at 1:04 AM Thomas Mortagne < > thomas.morta...@xwiki.com> > > wrote: > > > > > OK should probably be documented in the 1.7->2.0 upgrade then because > > > it was not very obvious before that it was deprecated since it was the > > > default behavior not something you could disable. > > > > > > So next question I guess is would you consider having it disabled by > > > default and be able to enable with a configuration like it was done > > > for other things in 2.1/2.2 ? I have to try :) > > > > > > On Tue, Jan 14, 2020 at 7:24 PM Nathan Bubna <nbu...@gmail.com> wrote: > > > > > > > > This is a planned change. Implicit local scope was deprecated in 1.7 > for > > > > removal in 2.0 and was replaced with explicit scope control objects. > > > > > > > > https://velocity.apache.org/engine/devel/upgrading.html#upgrading-from-velocity-16x-to-velocity-17x > > > > > > > > On Tue, Jan 14, 2020 at 9:29 AM Thomas Mortagne < > > > thomas.morta...@xwiki.com> > > > > wrote: > > > > > > > > > Took me a while to understand it but I actually found a new > difference > > > > > in the way Velocity deal with variables set inside a macro. > > > > > > > > > > In 1.7 by default (i.e. without "velocimacro.context.localscope") > > > > > variables set in a macro end up in both the global context and a > local > > > > > one and when accessing this variable later in the macro the local > > > > > value is used retrieved (all this happen in ProxyVMContext). > > > > > > > > > > This means that if the macro also calls some method that update the > > > > > global VelocityContext it won't have any effect on the following > macro > > > > > code because that change will be hidden by the local context. > While I > > > > > did not debugged on Velocity 2.2-SNAPSHOT , the result I get > suggest > > > > > that there is no local context in macros anymore or at least it's > not > > > > > used the same way in this use case. > > > > > > > > > > The use case in which I noticed this behavior change it is a macro > > > > > which initialization some variable, calls an API which execute > another > > > > > Velocity template (a kind of include in other words) and then use > the > > > > > previously set variable. What happen after upgrade is that the > > > > > variable is "broken" by the other template (because the template > use a > > > > > variable with the same name for totally unrelated reasons). > > > > > > > > > > It does not affect XWiki Standard much (that's why I only found it > > > > > now) and I'm not sure if it used to work only by luck or if whoever > > > > > wrote that use case really was counting on macro variables to be > > > > > "protected" but it might have a huge effect on some extensions. > > > > > > > > > > How do you feel about this change and do you want me to create a > jira > > > > > issue about this behavior change ? > > > > > > > > > > On Thu, Jan 9, 2020 at 2:50 PM Thomas Mortagne > > > > > <thomas.morta...@xwiki.com> wrote: > > > > > > > > > > > > False alarm actually I think, sorry for the noise. > > > > > > > > > > > > On Thu, Jan 9, 2020 at 2:21 PM Thomas Mortagne > > > > > > <thomas.morta...@xwiki.com> wrote: > > > > > > > > > > > > > > Unfortunately it seems the fix is not complete (commented on > the > > > Jira > > > > > issue). > > > > > > > > > > > > > > On Tue, Jan 7, 2020 at 3:55 PM Thomas Mortagne > > > > > > > <thomas.morta...@xwiki.com> wrote: > > > > > > > > > > > > > > > > So that one was on me after all. > > > > > > > > > > > > > > > > XWiki is a big beast and I might have missed some edge case > but > > > let's > > > > > > > > say +1 for a RC. > > > > > > > > > > > > > > > > On Tue, Jan 7, 2020 at 11:23 AM Thomas Mortagne > > > > > > > > <thomas.morta...@xwiki.com> wrote: > > > > > > > > > > > > > > > > > > There is one more thing that I want to debug today > (hopefully > > > > > answer > > > > > > > > > before 6pm GMT+1) to check if it's a side effect of this > bug, > > > > > another > > > > > > > > > regression or a mistake I made when I upgraded since a lot > of > > > APIs > > > > > > > > > changed (it's not an obvious error, just a slight > difference > > > in the > > > > > > > > > result but there are quite a few layers of abstractions to > > > debug > > > > > > > > > between the result and the Velocity engine). > > > > > > > > > > > > > > > > > > On Tue, Jan 7, 2020 at 10:56 AM Claude Brisson > > > > > > > > > <cla...@renegat.net.invalid> wrote: > > > > > > > > > > > > > > > > > > > > Fixed. Thanks for the report. > > > > > > > > > > > > > > > > > > > > Thomas, do you think you may have any other subtle xwiki > bug > > > to > > > > > report > > > > > > > > > > before I push out the next RC? > > > > > > > > > > > > > > > > > > > > On 20-01-03 12 h 31, Thomas Mortagne wrote: > > > > > > > > > > > Hi sorry me again, > > > > > > > > > > > > > > > > > > > > > > I just hit > > > https://issues.apache.org/jira/browse/VELOCITY-924. > > > > > > > > > > > > > > > > > > > > > > On Wed, Jan 1, 2020 at 11:12 PM Claude Brisson > > > > > > > > > > > <cla...@renegat.net.invalid> wrote: > > > > > > > > > > >> The test build of Velocity Engine 2.2 RC4 is > available. > > > > > > > > > > >> > > > > > > > > > > >> No determination as to the quality ('alpha,' 'beta,' > or > > > 'GA') > > > > > of > > > > > > > > > > >> Velocity Engine 2.1 RC3 has been made, and at this > time > > > it is > > > > > simply a > > > > > > > > > > >> "test build". We welcome any comments you may have, > and > > > will > > > > > take all > > > > > > > > > > >> feedback into account if a quality vote is called for > this > > > > > build. > > > > > > > > > > >> > > > > > > > > > > >> Release notes: > > > > > > > > > > >> > > > > > > > > > > >> * > > > > > > > > > > >> > > > > > > > > > https://dist.apache.org/repos/dist/dev/velocity/velocity-engine/2.2/release-notes.html > > > > > > > > > > >> > > > > > > > > > > >> Distribution: > > > > > > > > > > >> > > > > > > > > > > >> * > > > > > > https://dist.apache.org/repos/dist/dev/velocity/velocity-engine/2.2/ > > > > > > > > > > >> > > > > > > > > > > >> Maven 2 staging repository: > > > > > > > > > > >> > > > > > > > > > > >> * > > > > > > > > > > >> > > > > > > > > > https://repository.apache.org/content/repositories/orgapachevelocity-1032/ > > > > > > > > > > >> > > > > > > > > > > >> Documentation: > > > > > > > > > > >> > > > > > > > > > > >> * https://velocity.apache.org/engine/2.2/ > > > > > > > > > > >> > > > > > > > > > > >> Sources: > > > > > > > > > > >> > > > > > > > > > > >> * > > > > > https://svn.apache.org/repos/asf/velocity/engine/tags/2.2/ > > > > > > > > > > >> > > > > > > > > > > >> Release Candidates History: > > > > > > > > > > >> > > > > > > > > > > >> * RC1 Initial RC > > > > > > > > > > >> > > > > > > > > > > >> * RC2 > > > > > > > > > > >> - added BigInteger and BigDecimal implicit > conversions > > > > > > > > > > >> - [VELOCITY-923] fixed a parser regression for > `$foo||` > > > > > > > > > > >> - [VELOCITY-904] fixed two corner case bugs for the > > > > > > > > > > >> velocimacro.arguments.preserve_literals backward > > > > > compatibility flag > > > > > > > > > > >> - fixed engine and dependency versions in README > and > > > > > mention the > > > > > > > > > > >> parser customization feature in the *building* section > > > > > > > > > > >> - nicified README links > > > > > > > > > > >> - upgraded surfire plugin version from 2.19.1 to > 2.22.1 > > > > > > > > > > >> - upgraded maven-jar-plugin from 3.1.1 to 3.2.2 > > > > > > > > > > >> - added version 1.2 for extra-enforcer-rules > > > > > > > > > > >> - upgraded maven-javadoc-plugin from 3.1.0 to 3.1.1 > > > > > > > > > > >> - upgraded findbugs-maven-plugin from 3.0.4 to > 3.0.5 > > > > > > > > > > >> - upgraded maven-release-plugin from *unspecified* > to > > > > > 3.0.0-M1 > > > > > > > > > > >> - added a new templatized static class > > > > > > > > > > >> org.apache.velocity.runtime.VelocityEngineVersion.java > > > > > > > > > > >> - use the File Separator control character to mark > the > > > end > > > > > of stream > > > > > > > > > > >> for the parser (instead of the zero-width space char) > > > > > > > > > > >> - reviewed packaging of engine examples (refreshed > > > > > content, plus made > > > > > > > > > > >> them as a standalone zip file with readme, shell > scripts, > > > > > dependencies > > > > > > > > > > >> and examples sources rather than a meaningless > standalone > > > pom > > > > > next to a > > > > > > > > > > >> jar without explanations...) > > > > > > > > > > >> > > > > > > > > > > >> * RC3 > > > > > > > > > > >> - [VELOCITY-904] fixed yet another corner case bugs > > > for the > > > > > > > > > > >> velocimacro.arguments.preserve_literals backward > > > > > compatibility flag > > > > > > > > > > >> - upgraded SLF4J from 1..7.28 to 1.7.30 > > > > > > > > > > >> > > > > > > > > > > >> * RC4 > > > > > > > > > > >> - [VELOCITY-904] fixed a regression introduced in > RC3 > > > > > > > > > > >> > > > > > > > > > > >> > > > > > > > > > > >> -- > > > > > > > > > > >> Claude > > > > > > > > > > >> > > > > > > > > > > >> > > > > > > > > > > >> > > > > > > > > > > >> > > > > > > --------------------------------------------------------------------- > > > > > > > > > > >> To unsubscribe, e-mail: > > > dev-unsubscr...@velocity.apache.org > > > > > > > > > > >> For additional commands, e-mail: > > > dev-h...@velocity.apache.org > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > --------------------------------------------------------------------- > > > > > > > > > > To unsubscribe, e-mail: > dev-unsubscr...@velocity.apache.org > > > > > > > > > > For additional commands, e-mail: > > > dev-h...@velocity.apache.org > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > > > Thomas Mortagne > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > > Thomas Mortagne > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > Thomas Mortagne > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > Thomas Mortagne > > > > > > > > > > > > > > > > > > > > -- > > > > > Thomas Mortagne > > > > > > > > > > > --------------------------------------------------------------------- > > > > > To unsubscribe, e-mail: dev-unsubscr...@velocity.apache.org > > > > > For additional commands, e-mail: dev-h...@velocity.apache.org > > > > > > > > > > > > > > > > > > > > > > -- > > > Thomas Mortagne > > > > > > --------------------------------------------------------------------- > > > To unsubscribe, e-mail: dev-unsubscr...@velocity.apache.org > > > For additional commands, e-mail: dev-h...@velocity.apache.org > > > > > > > > > > -- > Thomas Mortagne > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@velocity.apache.org > For additional commands, e-mail: dev-h...@velocity.apache.org > >
