On Sep 4, 2009, at 10:30 AM, Thomas Mortagne wrote: But would you be ok to make this change in 2.1M1? Or would you prefer we live with it and instead document that it doesn't execute in the document's contex (and instead add a new method that does)? It's much simpler for sure but keeps adding to the mess. We could decide to keep adding to the mess and fix it when we rewrite that part in some far future.

Thanks -Vincent _______________________________________________ devs mailing list devs(a)xwiki.org http://lists.xwiki.org/mailman/listinfo/devs

Greetings from the far future! No, the new model is sadly not here yet, but we have a shiny new UI :) Bumping this old thread since I`ve stumbled upon this issue recently and it took me a while to understand what the actual problem is (thanks Thomas!) and I feel it's something we really need to get fixed as soon as possible. Thomas was suggesting we add this on the list for the 7.x Roadmap and I`m obviously +1 for that. There is the matter of backwards compatibility, that we need to still address, as either to break it or keep it. My take would be to break it, since I consider it to be bad code to depend on this broken behavior in the first place and that huge security hole we keep dragging along needs to be plugged abruptly. WDYT?

Thanks, Eduard On Fri, Sep 4, 2009 at 11:56 AM, Thomas Mortagne wrote: > On Fri, Sep 4, 2009 at 10:39, Vincent Massol wrote: > > > > On Sep 4, 2009, at 10:30 AM, Thomas Mortagne wrote: > > > >> On Fri, Sep 4, 2009 at 10:07, Vincent Massol > >> wrote: > >>> Hi, > >>> > >>> Im trying to fix http://jira.xwiki.org/jira/browse/XWIKI-4274 > >>> > >>> Basically if you do > >>> $xwiki.getDocument("someDoc").getRenderedContent() > >>> it'll get executed in the context of the current doc which I believe > >>> is wrong especially since other signatures of getRenderedContent() > >>> execute in the target document's context. > >>> > >>> I have fixed this locally but found that admin.vm for example is > >>> assuming that getRenderedContent() will get executed in the context > >>> of > >>> the calling doc (i.e. XWiki.Import when doing an import for example). > >>> FYI the chain flow is admin.vm -- getRenderedContent() --> > >>> XWiki.AdminSheet --> XWiki.AdminImportSheet --> importinline.vm, > >>> which > >>> requires the current doc to be XWiki.Import (to get/put attachments > >>> from/to it). > >>> I can fix this easily using a new getRenderedContent signature I've > >>> introduced. > >>> > >>> However I'm wondering if we have other places that incorrectly use > >>> getRenderedContent() and assume it won't be rendered in the context > >>> of > >>> the target document. > >>> > >>> Is this change too dangerous to make? If not know, we'll need to it > >>> quickly (2.1M1?) since it's an important bug IMO. > >> > >> Yes it's a dangerous fix I think, getRenderedContent() is really not > >> an hidden method so changing its behavior that much even if it's a fix > >> is dangerous anyway. > > > > But would you be ok to make this change in 2.1M1? > > > > Or would you prefer we live with it and instead document that it > > doesn't execute in the document's contex (and instead add a new method > > that does)? It's much simpler for sure but keeps adding to the mess. > > We could decide to keep adding to the mess and fix it when we rewrite > > that part in some far future. > > Actually i was mainly thinking that we will rewrite all that in the > new model anyway so for me one way or another it's temporary code so i > think i would be more in favor of not changing the behavior of the > current getRenderedContent() and instead add new one for backward > compatibility. > > > > > Thanks > > -Vincent