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

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

Conversation

AndoniLarz
Copy link

@AndoniLarz AndoniLarz commented Apr 1, 2023

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

TODO :

  • Fix conflicts (by rebasing)
  • Edit the context option name (enable_cdata_wrapping => cdata_wrapping)
  • Wait for the reviews

@carsonbot carsonbot added this to the 6.3 milestone Apr 1, 2023
@OskarStark OskarStark added the Waiting Code Merge Docs for features pending to be merged label Apr 2, 2023
@carsonbot carsonbot modified the milestones: 6.3, next Apr 2, 2023
nicolas-grekas added a commit to symfony/symfony that referenced this pull request Oct 11, 2023
…option (AndoniLarz)

This PR was merged into the 6.4 branch.

Discussion
----------

[Serializer] Add `XmlEncoder::CDATA_WRAPPING` context option

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

Usage example :
```php
$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
<?xml version="1.0"?>
<response><Hello>Paul &amp; Martha &lt;or Me&gt;</Hello></response>
```

instead of

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

---

As stated in [the following comment from PHP.net](https://www.php.net/manual/fr/domdocument.createtextnode.php#116351) : PHP automatically handles the escape when calling `DOMDocument::createTextNode` which is done in [::appendText](https://github.com/symfony/symfony/blob/6.3/src/Symfony/Component/Serializer/Encoder/XmlEncoder.php#L193).
`::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](https://www.w3schools.com/xml/xml_syntax.asp#:~:text=/note%3E-,Entity%20References,-Some%20characters%20have) 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 `['<', '>', '&']`)

---
- [X] Create the documentation PR
- [X] Check Fabbot feedback to make sure code is well written

Commits
-------

049982d [Serializer] Add `XmlEncoder::CDATA_WRAPPING` context option
@AndoniLarz AndoniLarz changed the base branch from 6.3 to 6.4 April 10, 2024 09:56
@xabbuh xabbuh removed the Waiting Code Merge Docs for features pending to be merged label Apr 11, 2024
@xabbuh xabbuh modified the milestones: next, 6.4 Apr 11, 2024
@AndoniLarz AndoniLarz force-pushed the feature/add-documentation-for-xml-encoder-cdata-wrapping-opt-out branch from 9437594 to f0a98ad Compare April 23, 2024 13:23
@AndoniLarz
Copy link
Author

Hi @xabbuh @OskarStark , I fixed the conflicts and updated the context option name following the code that has been merged.
Does it look good to you ?

``cdata_wrapping`` If set to false, will not wrap any value ``true``
containing one of the following characters (
``<``, ``>``, ``&``) in `a CDATA section`_ like
following : ``<![CDATA[...]]>``
============================== ================================================= ==========================

Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a versionadded directive, you can find some as example in other files

Copy link
Author

Choose a reason for hiding this comment

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

Done

@OskarStark OskarStark modified the milestones: 6.4, next Apr 29, 2024
@OskarStark
Copy link
Contributor

Please target 7.1 branch for now, thanks

@OskarStark OskarStark added the Waiting Code Merge Docs for features pending to be merged label Apr 29, 2024
@AndoniLarz
Copy link
Author

Please target 7.1 branch for now, thanks

Even though the feature has been added in 6.4 ?

@OskarStark
Copy link
Contributor

Even though the feature has been added in 6.4 ?

Ah sorry this is about another option. Sorry I mixed things up. No in this car 6.4 is fine, but a versionadded would be helpful at all:


.. versionadded:: 6.4

    The `cdata_wrapping` option was introduced in Symfony 6.4.

@OskarStark OskarStark modified the milestones: next, 6.4 Apr 29, 2024
@OskarStark OskarStark removed the Waiting Code Merge Docs for features pending to be merged label Apr 29, 2024
@AndoniLarz
Copy link
Author

Even though the feature has been added in 6.4 ?

Ah sorry this is about another option. Sorry I mixed things up. No in this car 6.4 is fine, but a versionadded would be helpful at all:


.. versionadded:: 6.4

    The `cdata_wrapping` option was introduced in Symfony 6.4.

Alright no problem. I'll make the changes soon

@OskarStark
Copy link
Contributor

Open for finishing this PR by handling my comments?

@AndoniLarz
Copy link
Author

Open for finishing this PR by handling my comments?

It should be good now 😄

@OskarStark OskarStark force-pushed the feature/add-documentation-for-xml-encoder-cdata-wrapping-opt-out branch from 77e3a00 to 705d5d2 Compare May 16, 2024 05:14
@OskarStark
Copy link
Contributor

OskarStark commented May 16, 2024

Thank you Andoni and congratulations on your first contribution to the Symfony documentation 🎉

@OskarStark OskarStark merged commit cb08b46 into symfony:6.4 May 16, 2024
3 checks passed
@AndoniLarz AndoniLarz deleted the feature/add-documentation-for-xml-encoder-cdata-wrapping-opt-out branch May 16, 2024 14:48
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.

None yet

4 participants