Skip to content

[Serializer] Add XmlEncoder::CDATA_WRAPPING context option #49893

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

Conversation

AndoniLarz
Copy link

@AndoniLarz AndoniLarz commented Apr 1, 2023

Q A
Branch? 6.4
Bug fix? no
New feature? yes
Deprecations? no
Tickets Fix #26168
License MIT
Doc PR symfony/symfony-docs#18155

Usage example :

$data = [
	'Hello' => 'Paul & Martha <or Me>',
];

return new Response(
    $this->serializer->serialize(
        $data,
        XmlEncoder::FORMAT,
        [
            XmlEncoder::CDATA_WRAPPING => false
        ]
    ),
    headers: [
        'Content-Type' => 'application/xml'
    ]
);

will output

<?xml version="1.0"?>
<response><Hello>Paul &amp; Martha &lt;or Me&gt;</Hello></response>

instead of

<?xml version="1.0"?>
<response><Hello><![CDATA[Paul & Martha <or Me>]]></Hello></response>

As stated in the following comment from PHP.net : PHP automatically handles the escape when calling DOMDocument::createTextNode which is done in ::appendText.
::appendText is always called if XmlEncoder::CDATA_WRAPPING => false.


Questions:

  • Should we allow for custom XPath to be wrapped in CDATA while others are simply escaped ?
  • Should we allow for a callback function to be used in place of a boolean (callable(string $val): bool)
  • W3Schools#Entity References states that ' and " might also be escaped. Based on this should we instead provide a list of characters to escape/wrap ? (currently it would be set to ['<', '>', '&'])

  • Create the documentation PR
  • Check Fabbot feedback to make sure code is well written

@carsonbot
Copy link

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has a contribution guide which I suggest you to read.

In short:

  • Always add tests
  • Keep backward compatibility (see https://symfony.com/bc).
  • Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
  • Features and deprecations must be submitted against the 6.3 branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

@carsonbot carsonbot changed the title [WIP] [Serializer] Add XmlEncoder CDATA wrapping opt out [Serializer] [WIP] Add XmlEncoder CDATA wrapping opt out Apr 1, 2023
@AndoniLarz AndoniLarz changed the title [Serializer] [WIP] Add XmlEncoder CDATA wrapping opt out [Serializer] [XmlEncoder] Add XmlEncoder CDATA wrapping opt out Apr 1, 2023
@AndoniLarz
Copy link
Author

Failing checks seem unrelated to my changes

@astehlik
Copy link

astehlik commented Sep 8, 2023

I tested the changes from the PR and they work like a charm 👍

@nicolas-grekas nicolas-grekas changed the title [Serializer] [XmlEncoder] Add XmlEncoder CDATA wrapping opt out [Serializer] Add XmlEncoder::ENABLE_CDATA_WRAPPING context option Oct 11, 2023
@nicolas-grekas nicolas-grekas changed the title [Serializer] Add XmlEncoder::ENABLE_CDATA_WRAPPING context option [Serializer] Add XmlEncoder::CDATA_WRAPPING context option Oct 11, 2023
@nicolas-grekas nicolas-grekas force-pushed the ticket_26128-add-xml-encoder-cdata-wrapping-opt-out branch from 7b9af66 to c80761e Compare October 11, 2023 14:18
@nicolas-grekas nicolas-grekas force-pushed the ticket_26128-add-xml-encoder-cdata-wrapping-opt-out branch from c80761e to 049982d Compare October 11, 2023 14:22
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

I fixed the remaining comments and renamed the option to CDATA_WRAPPING

@nicolas-grekas
Copy link
Member

Thank you @AndoniLarz.

@nicolas-grekas nicolas-grekas merged commit 9537ae2 into symfony:6.4 Oct 11, 2023
This was referenced Oct 21, 2023
OskarStark added a commit to symfony/symfony-docs that referenced this pull request May 16, 2024
…CDATA wrapping opt-out context option (AndoniLarz)

This PR was squashed before being merged into the 6.4 branch.

Discussion
----------

[Serializer] Add documentation about a new XmlEncoder CDATA wrapping opt-out context option

Add documentation of symfony/symfony#49893 which has been merged in 6.4.

TODO :
- [x] Fix conflicts (by rebasing)
- [x] Edit the context option name (`enable_cdata_wrapping` => `cdata_wrapping`)
- [x] Wait for the reviews

Commits
-------

705d5d2 [Serializer] Add documentation about a new XmlEncoder CDATA wrapping opt-out context option
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Serializer][XmlEncoder] Don't wrap content in CDATA
5 participants