Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

When calling the html serializer pass an encoding #239

Merged
merged 1 commit into from
May 2, 2024
Merged

When calling the html serializer pass an encoding #239

merged 1 commit into from
May 2, 2024

Conversation

ale-rt
Copy link
Member

@ale-rt ale-rt commented Apr 30, 2024 •
edited
Loading

Fixes #238

Without passing the encoding the test fails with:

AssertionError: '<!DOCTYPE html>\n<html>\n<body>\n<div>à</div>\n</body>\n</html>' != '<!DOCTYPE html>\n<html>\n<body>\n<div>Ã\xa0</div>\n</body>\n</html>'
  <!DOCTYPE html>
  <html>
  <body>
- <div>à</div>
?      ^
+ <div>à</div>
?      ^^
  </body>
  </html>

@mister-roboto
Copy link

@ale-rt thanks for creating this Pull Request and helping to improve Plone!

TL;DR: Finish pushing changes, pass all other checks, then paste a comment:

@jenkins-plone-org please run jobs

To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass, but it takes 30-60 min. Other CI checks are usually much faster and the Plone Jenkins resources are limited, so when done pushing changes and all other checks pass either start all Jenkins PR jobs yourself, or simply add the comment above in this PR to start all the jobs automatically.

Happy hacking!

@ale-rt
Copy link
Member Author

ale-rt commented Apr 30, 2024

@jenkins-plone-org please run jobs

gforcada
gforcada reviewed May 1, 2024
View reviewed changes
src/plone/app/theming/transform.py
@@ -120,7 +121,9 @@ def parseTree(self, result):
return None

try:
return getHTMLSerializer(result, pretty_print=False)
return getHTMLSerializer(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's beyond the scope of this fix, but I was doing a transform yesterday and changed getHTMLSerializer from repoze.xmliter with lxml.etree.HTMLParser. Should we actually move to that one?

repoze.itertools does not look much taken care of 😓

And actually it is just a tiny wrapper to lxml...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks a nice idea to get rid of a dependency.
I created #240. I added the milestone 6.1 there, I would avoid doing that change for Plone 6.0 right now.

gforcada
gforcada approved these changes May 1, 2024
View reviewed changes
Copy link
Member

@gforcada gforcada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ale-rt ale-rt mentioned this pull request May 2, 2024
Open
@ale-rt ale-rt merged commit ebaa95f into master May 2, 2024
13 checks passed
@ale-rt ale-rt deleted the 238-bugfix branch May 2, 2024 06:34
@mauritsvanrees
Copy link
Member

I could not reproduce problems in a client project, so it probably depends on how exactly you use plone.app.theming. Anyway, with your code I see no problems either, so LGTM.

I have released plone.app.theming 5.0.9 and will include it in the coredev buildouts.

@petschki
Copy link
Member

petschki commented May 7, 2024 •
edited
Loading

Ah, thank you! I encountered encoding problems here with the package collective.collectionfilter when upgradeing from Plone 6.0.10.1 -> 6.0.11, because this calls the @@portlet-renderer via XHR and got unencoded characters back. I can confirm this is solved with updateing to plone.app.theming=5.0.9

@petschki
Copy link
Member

petschki commented May 8, 2024

You see this currently on https://classic.demo.plone.org/de/folder_contents in the german toolbar:

Bildschirmfoto 2024-05-08 um 16 53 46

@mauritsvanrees maybe we can get a 6.0.11.1 release with this update here?

@mauritsvanrees
Copy link
Member

Aha, now I finally see the problem in practice.
The same problem exists even in English, where 'Add new...' is show as 'Add new�':

Screenshot 2024-05-08 at 17 57 40

Let me see about a new release indeed. :-/

@mauritsvanrees
Copy link
Member

I am surprised that this is only a problem in folder contents, and not on view.

@mauritsvanrees
Copy link
Member

6.0.11.1 is available on dist.plone.org:
https://dist.plone.org/release/6.0.11.1/

k-probst pushed a commit to OpenBfS/dokpool-plone that referenced this pull request Jul 23, 2024
@pbauer
Remove old pinn and fix #5643 with plone/plone.app.theming#239
ee24714
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gforcada gforcada gforcada approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support lxml 5
5 participants
@ale-rt @mister-roboto @mauritsvanrees @petschki @gforcada