p.a.blocks transforms change default behavior of ajax responses #5

Closed
rnixx opened this Issue Oct 26, 2013 · 22 comments

5 participants

@rnixx
Plone Foundation member

After installing plone.app.blocks, all AJAX HTML responses are wrapped within html / body tags, which differs how zope normally behaves and thus may break existing addons.

The responsible code line is here

https://github.com/plone/plone.app.blocks/blob/master/plone/app/blocks/transform.py#L84

Some unwanted side effect is described here

collective/collective.cover#331

how can we ensure the responsible transform only considers requests referring to p.a.blocks?

@rnixx
Plone Foundation member

Uplcoming questions about this issue

what's the reason of wrapping at all -> the ESI stuff?
where are the results processed further on the client side?

@rnixx
Plone Foundation member

As described here

https://github.com/plone/plone.transformchain/blob/master/plone/transformchain/interfaces.py#L15

binding the transform to the basic tile interface might do the job

i created a branch with this changes

9b6742e

and run the tests. Beside 2 failing tests in the main branch one more tests is failing.

If this is the desired fix for the issue, i'll fix the tests and merge the branch. comments appreciated

@vangheem
Plone Foundation member

I was just experiencing this issue.

Your fix is good and I think you should merge. Thanks!

@davisagli
Plone Foundation member

I don't understand the fix. The transforms are designed to operate on things that reference tiles (i.e. potentially any text/html response), not on the tiles themselves. Doesn't this change completely disable tile rendering?

@vangheem
Plone Foundation member

oh right.... Then how do we stop this?

@vangheem
Plone Foundation member

How I got around it is making my responses content-type text/xml and then the transform won't run on them...

@davisagli
Plone Foundation member

I think depending on a text/html response to not contain html and body tags is wrong, because without them it's not valid HTML. Seems like the response should be given a different mimetype. text/x-not-quite-html or something.

But if it's really necessary maybe the transform can detect when the original response doesn't have these tags, and not transform in that case.

@rnixx
Plone Foundation member

@davisagli referring to the documentation of ITransform, the multi adapter applies to the last element returned by traversal, which should be a view. that was the reason i thought binding to IBaseicTile does the job.

Referring to invalid HTML. Basically this argument does not matter at all. plone.app.blocks changes the way responses are delivered in general, thus forcing everybody else to change their code. Not everything is tiles or blocks, i even not understand yet what's the concrete reason for this transforms.

Some may enlighten me ;)

@davisagli
Plone Foundation member

The whole point of plone.app.blocks is that it allows you to reference tiles or layouts from any html response. So yes, it therefore needs to process all html responses to find out if those things are referenced. That is by design. Not everything is a tile, but anything may include a tile.

If that's not acceptable, you can a) not use plone.app.blocks, set request['plone.app.blocks.disabled'] = True to disable the transforms for your request, b) return a mimetype that won't get transformed, or d) find a way to abort the transform and return the original response if no tiles or layouts were found

@rnixx
Plone Foundation member

i.e., if i have a valid dom tree in the browser. and write some kind of ajaxified application which alters just a small part of the document (maybe a div or similar), and this small part gets delivered by an ordinary browser view, i don not see how this invalidates the overall document. I also don't know any specification which requires a valid HTML document structure for ajax snippets.

referring solution a. this might get difficult in future, i.e. i ran into this issue after installing collective.cover, and the decision using it is not necessarily made by the developer

as said above, p.a.blocks changes the way zope browser views behave by default, and that's a very very bad thing

@rnixx
Plone Foundation member

another question - whats wrong with doing the reference detection stuff without wrapping the response in a HTML document and leave document validity elsewhere?

@davisagli
Plone Foundation member

plone.app.blocks was developed with Deco in mind, where tiles can be included anywhere. But for collective.cover tiles only appear in the view of a cover. So perhaps collective.cover should not include the plone.app.blocks zcml but should instead register just the ParseXML and IncludeTiles transforms for covers only.

Turning the response into valid html is not an explicit goal of plone.app.blocks, but is something that happens incidentally as a result of reserializing the lxml tree with the html serializer after processing it to insert tile contents, merge panels, and include the layout. So it can't be avoided in the case where the response actually does need to be modified.

@rnixx
Plone Foundation member

it can be avoided with a custom HTML serialzer then (especially since wrapping is not a requirement for working blocks/tiles).

The point is still that p.a.blocks changes default zope behavior == BAD.

No matter how a workaround might look like right now. This should definitely be fixed. In my case, it breaks some of my own addon products, but i bet there's also lots of other 3rd party stuff effected.

@rnixx
Plone Foundation member

This is the html serializer of lxml.

https://github.com/lxml/lxml/blob/master/src/lxml/html/__init__.py#L1524

should be possible to avoid wrapping by several control flags, need to experiment a little bit

and here's the serializer creating utility. the serializer might even be initialized without using this utility.

https://github.com/repoze/repoze.xmliter/blob/master/repoze/xmliter/utils.py#L21

@jensens
Plone Foundation member

@davisagli If you write "Turning the response into valid html is not an explicit goal of plone.app.blocks", this is an implicit change of Plones default behavior.

-> So, this is a Regression Bug and need to be fixed.

Also there is no need to change the markup around, this may have other side effects too.

I agree with @rnixx that a sane behavior can be triggered easily by using a different, less immersive serializer.

@vangheem
Plone Foundation member

check out my comment to your pull request: #7

We don't want to be parsing the dom multiple times--we need to do this where the parsed dom can be re-used.

@rnixx
Plone Foundation member

as far as i can see the tree gets cached nowhere, thus always reparsed. correct me if i'm wrong

@vangheem
Plone Foundation member

Look at: https://github.com/repoze/repoze.xmliter/blob/master/repoze/xmliter/utils.py#L11

The dom is not re-parsed if it's already done. I think we try to use getXMLSerializer everywhere for this reason.

Moreover, can you be sure that https://github.com/plone/plone.app.blocks/blob/29e02adc535383321b38f28259b56947390ce47b/plone/app/blocks/transform.py#L83 is always an iterable string at this point? What if there was something earlier in the transform that already parsed it to XMLSerializer?

@rnixx
Plone Foundation member

i check for XMLSerializer instance now in my branch. but if the utility function gets used in other places as well and it is possible that a transform hooks up before the modified one, i think it would be better to create another utility function where the tree gets created the way i do now

@datakurre
Plone Foundation member

I like @davisagli's idea of registering blocks-transformation only for the types (e.g. cover), which really need it (until we have something as ubiquitous as deco). At least it'd be the easiest way.

@datakurre
Plone Foundation member

After Mosaic Sprint, I'd say that this issue is almost wontfix, but let's not close this yet:

In collective.cover, the most appropriate fix would be to not include p.a.blocks' zcml automatically, but only register the required transformations for c.cover context.

In deco / p.a.mosaic, it's a main feature to support transformations anywhere. But once we have some more tests in p.a.mosaic, I'll look into fixing by making transformations opt-in (instead of opt-out) in mosaicsprint-branch. Yet, because it would break all existing blocks-depending packages, I can only do it in sync with fixing also p.a.mosaic and won't backport the fix into the current (old) master branch.

@datakurre
Plone Foundation member

This is now fixed for mosaicsprint-branch (which will probably be released as 2.0.0 and renamed to new master once p.a.mosaic is ready) in #12

In the solution, transformed view should explicitly implement IBlocksTransformEnabled to trigger Blocks' transforms. This is already implemented for the default view for content with layout aware behavior. For non layout aware views this could be done in main_template-view (available in Plone 5 and p.a.mosaic).

@jensens jensens added the Bug label Mar 5, 2015
@jensens jensens closed this Apr 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment