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] Fix XML attributes not added on empty node #52589

Merged
merged 1 commit into from
Nov 23, 2023

Conversation

mtarld
Copy link
Contributor

@mtarld mtarld commented Nov 15, 2023

Q A
Branch? 5.4
Bug fix? yes
New feature? no
Deprecations? no
Issues Fix #52385
License MIT

Add XML namespace attributes on node whether they have chlidren or not.

@mtarld
Copy link
Contributor Author

mtarld commented Nov 15, 2023

fabbot errors are not related to that PR.

@nicolas-grekas
Copy link
Member

@mtarld can you please have a look at #52401 and help us decide which approach we should merge? It looks like the test cases are the same, right?

@mtarld
Copy link
Contributor Author

mtarld commented Nov 20, 2023

I think that there is an issue with #52401.

Indeed, the following XML:

<?xml version="1.0"?>
<response xmlns="http://www.w3.org/2005/Atom" xmlns:app="http://www.w3.org/2007/app" app:foo="bar">
</response>

will be decoded as:

[
    '@app:foo' => 'bar',
    '#' => '',
]

whereas it should in my opinion be decoded as:

[
    '@xmlns' => 'http://www.w3.org/2005/Atom',
    '@xmlns:app' => 'http://www.w3.org/2007/app',
    '@app:foo' => 'bar',
    '#' => '',
]

otherwise we lose the XML namespaces information.

But this is valid only if we do want to care about XML namespaces, of course.

@fabpot
Copy link
Member

fabpot commented Nov 23, 2023

Thank you @mtarld.

@fabpot fabpot merged commit 1199090 into symfony:5.4 Nov 23, 2023
7 of 11 checks passed
@mtarld mtarld deleted the fix/xml-namespaces branch November 23, 2023 07:27
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