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.

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

pass matching mime type to xmldom; test [dynamicxml] #10830

Merged
merged 2 commits into from
Jan 21, 2025

Conversation

karfau
Copy link
Contributor

@karfau karfau commented Jan 19, 2025

As discussed in #10827, If the Content-Type header contains one of the mime types supported by DOMParser, the first matching mime type will be used instead of text/xml.

The default is still text/xml for cases when the header is not present or none of the mime types from the list are present.

Copy link
Contributor

github-actions bot commented Jan 19, 2025

Warnings
⚠️ This PR modified service code for dynamic but not its test code.
That's okay so long as it's refactoring existing code.
Messages
📖 ✨ Thanks for your contribution to Shields, @karfau!

Generated by 🚫 dangerJS against cd4053e

@karfau
Copy link
Contributor Author

karfau commented Jan 19, 2025

I did those changes from the github UI so far.
I'm going to check out the code and resolve the issues.

I tried to guess the Dynamic service from the folder structure and what I read in the contribution guide. But is seems to be wrong, since there is no such service, or at least there are no tests for it. Is there any service I need to add to the PR title?

@karfau karfau changed the title [Dynamic] pass matching mime type to xmldom pass matching mime type to xmldom Jan 19, 2025
If the `Content-Type` header contains one of the mime types supported by `DOMParser`, the first matching mime type will be used instead of `text/xml`.

The default is still `text/xml` for cases when the header is not present or none of the mime types from the list are present.
@karfau karfau marked this pull request as ready for review January 19, 2025 17:11
@karfau
Copy link
Contributor Author

karfau commented Jan 19, 2025

From my perspective it looks as if it's ready for review.
If you want me to add tests those changes or enable certain service tests in the PR title, let me know.

@chris48s chris48s changed the title pass matching mime type to xmldom pass matching mime type to xmldom; test [dynamicxml] Jan 19, 2025
@chris48s chris48s added the service-badge New or updated service badge label Jan 19, 2025
@chris48s
Copy link
Member

Do any of the other values defined in MIME_TYPE (application/xml, application/xhtml+xml, image/svg+xml) trigger custom parser behaviour?

@karfau
Copy link
Contributor Author

karfau commented Jan 19, 2025

Update I read your question again: The short answer is yes.

There are the following affected areas:

  • parsing rules and error vs warning in XML vs HTML, XML fails on not well formed input while HTML might just report a warning
  • default HTML namespace in XHTML and HTML
  • entity handling (I need to check, but I think it's connected with the HTML namespace)
  • rendering to string and the so called normalization, also something of which I would need to look up the details of

But from my memory XHTML and HTML have (slightly different) special handling, especially regarding namespaces, everything else is currently just treated as XML.

Why?

@chris48s
Copy link
Member

I was mainly thinking about how it makes sense to test this tbh.

What I've done for this PR is factored this out into a function and just added some tests for the content type parsing. I think when we upgrade to a version of xmldom with a fix for #10827 I will add a service test for an html document with a lowercase doctype as an integration test for it.

Thanks

@chris48s chris48s enabled auto-merge January 21, 2025 19:51
@chris48s chris48s added this pull request to the merge queue Jan 21, 2025
Merged via the queue into badges:master with commit 007a7e6 Jan 21, 2025
23 checks passed
@karfau karfau deleted the patch-1 branch January 21, 2025 20:58
@karfau
Copy link
Contributor Author

karfau commented Jan 21, 2025

Great, your refactoring and tests make a lot of sense to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
service-badge New or updated service badge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants