use html method to avoid autoclosed tags - Githubissues

plone / Products.PortalTransforms

Provides MIME types based transformation chains on Plone Archetypes contents

4 stars 15 forks source link

use html method to avoid autoclosed tags #43

Closed cekk closed 2 years ago

mister-roboto commented 3 years ago

@cekk thanks for creating this Pull Request and help improve Plone!

To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass.

Whenever you feel that the pull request is ready to be tested, either start all jenkins jobs pull requests by yourself, or simply add a comment in this pull request stating:

@jenkins-plone-org please run jobs

With this simple comment all the jobs will be started automatically.

Happy hacking!

cekk commented 3 years ago

@jenkins-plone-org please run jobs

cekk commented 3 years ago

This pr solves a problem that we are having with Volto.

It seems that Volto(React) is more strict with html tags than Plone and doesn't like auto closed tags.

lxml, without method="html" convert and empty tag into a self-closed one (for example ).

In Volto this generates pages with broken text because React cuts everything after a self-closed tag.

cekk commented 3 years ago

@jenkins-plone-org please run jobs

tisto commented 3 years ago

@jenkins-plone-org please run jobs

cekk commented 3 years ago

i'm fixing several tests because some of them are broken (full of auto-closed tags). I have a couple of questions:

what's the correct way to fix test on dependencies (other products that use transforms) and fix jenkins?
there are some tests that fails because of different encoding (https://jenkins.plone.org/job/pull-request-5.2-3.7/1656/testReport/junit/Products.PortalTransforms.tests.test_xss/TestXSSFilter/test_38/). Probably this is related to my changes but i don't know if this could lead to some encoding problems.

tisto commented 3 years ago

@cekk you could set up a PLIP job and configuration. Though, it is also possible to trigger the Jenkins PR jobs with a set of PRs. If you can provide us with such a set of PRs together with the proof that Jenkins is green, we can merge them all together. At least this is the way we did things in the past.

cekk commented 3 years ago

@jenkins-plone-org please run jobs

gotcha commented 2 years ago

@cekk Do you know what is missing to get this merged ?

We need the same fix for another problem: with the default xml method any \r is turned into an  entity, which breaks multiline script tags.

cekk commented 2 years ago

@gotcha basically fix the tests into other packages..i didn't had the time to do it and i forgot this ticket :(

gotcha commented 2 years ago

@mauritsvanrees What is the process from here ? The succeeding jobs depend on PRs on CMFPlone, ATCT and p.a.discussion.

https://github.com/plone/plone.app.discussion/pull/192 https://github.com/plone/Products.CMFPlone/pull/3455 https://github.com/plone/Products.ATContentTypes/pull/68 https://github.com/plone/plone.app.discussion/pull/193 https://github.com/plone/Products.CMFPlone/pull/3456

They are small changes to support change in safe_html.

I am happy to release PortalTransforms if this helps.

mauritsvanrees commented 2 years ago

I see everything is tested in the right combinations on Plone 5.2 and 6.0. I will go ahead and approve and merge everything. Jenkins will probably fail in between merges, but at the end all should be well. Thanks for taking care of this!

mauritsvanrees commented 2 years ago

All are merged now.