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] [XML] Ignore Process Instruction #22044

Merged
merged 1 commit into from Mar 21, 2017

Conversation

Projects
None yet
5 participants
@jsamouh
Copy link
Contributor

commented Mar 17, 2017

Q A
Branch? 2.7
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #22005
License MIT
Doc PR N/A

This Pull request ignores Process instruction data in XML for decoding the data.

@jsamouh jsamouh force-pushed the jsamouh:ticket_22005 branch from 2b8fc43 to 22be529 Mar 17, 2017

@greg0ire
Copy link
Contributor

left a comment

Thanks a lot for your PR! I suggested small fixes

'<item><title>title1</title></item><?item ignore-title ?><item><title>title2</title></item>'.
'<Barry><FooBar id="1"><Baz>Ed</Baz></FooBar></Barry></baz>'.
'<qux>1</qux>'.
'</response><?instrtuction <value> ?>'."\n";

This comment has been minimized.

Copy link
@greg0ire

greg0ire Mar 17, 2017

Contributor
  • You should use nowdoc here.
  • instrtuction => instruction

This comment has been minimized.

Copy link
@jsamouh

jsamouh Mar 17, 2017

Author Contributor

@greg0ire oups, fixed, thanks ^^

This comment has been minimized.

Copy link
@greg0ire

greg0ire Mar 17, 2017

Contributor

No problem, but what about the nowdoc?

This comment has been minimized.

Copy link
@jsamouh

jsamouh Mar 17, 2017

Author Contributor

@greg0ire can you tell me more about this ? really don't know that: What change do you expect ?

@jsamouh jsamouh force-pushed the jsamouh:ticket_22005 branch from 22be529 to 6cbcebf Mar 17, 2017

'<item><title>title1</title></item><?item ignore-title ?><item><title>title2</title></item>'.
'<Barry><FooBar id="1"><Baz>Ed</Baz></FooBar></Barry></baz>'.
'<qux>1</qux>'.
'</response><?instruction <value> ?>'."\n";

This comment has been minimized.

Copy link
@greg0ire

greg0ire Mar 17, 2017

Contributor

Here is how it could look in case you don't know nowdoc:

<?php
         return <<<'XML'
<?xml version="1.0"?>
<?xml-stylesheet type="text/xsl" href="/xsl/xmlverbatimwrapper.xsl"?>
<?display table-view?>
<?sort alpha-ascending?>
<response>
<foo>foo</foo>
<?textinfo whitespace is allowed ?>
<bar>a</bar><bar>b</bar>
<baz><key>val</key><key2>val</key2><item key="A B">bar</item>
<item><title>title1</title></item><?item ignore-title ?><item><title>title2</title></item>
<Barry><FooBar id="1"><Baz>Ed</Baz></FooBar></Barry></baz>
<qux>1</qux>
</response><?instruction <value> ?>
XML;
  • no quotes
  • no linebreaks
  • no concatenation
  • syntactic coloration
  • but it kind of breaks the indention

This comment has been minimized.

Copy link
@jsamouh

jsamouh Mar 17, 2017

Author Contributor

ah , I see ^^! I just do like this to be conform to the existing example in the file. If contributors approves, I will do it.

This comment has been minimized.

Copy link
@dunglas

dunglas Mar 21, 2017

Member

I also prefer the nowdoc syntax as suggested by @greg0ire. As an interesting side effect, it allows PHPStorm to highlight embedded document.

@@ -465,6 +473,23 @@ public function testDecodeEmptyXml()
$this->encoder->decode(' ', 'xml');
}
protected function getXmlPISource()

This comment has been minimized.

Copy link
@greg0ire

greg0ire Mar 17, 2017

Contributor

You probably can make this private, or at least final

This comment has been minimized.

Copy link
@jsamouh

jsamouh Mar 17, 2017

Author Contributor

@greg0ire same response. just want to be conform to the existing code. But in fact, you're right ^^

This comment has been minimized.

Copy link
@greg0ire

greg0ire Mar 17, 2017

Contributor

Oh ok great 👍 I hope they won't make you change all other examples for the sake of consistency :P

This comment has been minimized.

Copy link
@dunglas

dunglas Mar 21, 2017

Member

I think you can use a private method for a new method.

@nicolas-grekas nicolas-grekas added this to the 2.7 milestone Mar 20, 2017

@jsamouh

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2017

ping @nicolas-grekas . WDYT ? ^^

@dunglas

This comment has been minimized.

Copy link
Member

commented Mar 21, 2017

👍 (when the small comments will be fixed).

@jsamouh jsamouh force-pushed the jsamouh:ticket_22005 branch from 6cbcebf to 2a5427a Mar 21, 2017

@jsamouh

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2017

@dunglas , here it is :-)

@@ -465,6 +473,41 @@ public function testDecodeEmptyXml()
$this->encoder->decode(' ', 'xml');
}
private function getXmlPISource()

This comment has been minimized.

Copy link
@dunglas

dunglas Mar 21, 2017

Member

I would even get rid of this function and put the doc in testDecodeXMLWithProcessInstruction.

This comment has been minimized.

Copy link
@jsamouh

jsamouh Mar 21, 2017

Author Contributor

@dunglas done

@jsamouh jsamouh force-pushed the jsamouh:ticket_22005 branch from 2a5427a to 0c741f5 Mar 21, 2017

@fabpot

This comment has been minimized.

Copy link
Member

commented Mar 21, 2017

Thank you @jordscream.

@fabpot fabpot merged commit 0c741f5 into symfony:2.7 Mar 21, 2017

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Mar 21, 2017

bug #22044 [Serializer] [XML] Ignore Process Instruction (jordscream)
This PR was merged into the 2.7 branch.

Discussion
----------

[Serializer] [XML] Ignore Process Instruction

| Q             | A
| ------------- | ---
| Branch?       | 2.7
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #22005
| License       | MIT
| Doc PR        | N/A

This Pull request ignores Process instruction data in XML for decoding the data.

Commits
-------

0c741f5 [Serializer] [XML] Ignore Process Instruction

This was referenced Apr 4, 2017

sandergo90 pushed a commit to sandergo90/symfony that referenced this pull request Jul 4, 2019

bug symfony#22044 [Serializer] [XML] Ignore Process Instruction (jord…
…scream)

This PR was merged into the 2.7 branch.

Discussion
----------

[Serializer] [XML] Ignore Process Instruction

| Q             | A
| ------------- | ---
| Branch?       | 2.7
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | symfony#22005
| License       | MIT
| Doc PR        | N/A

This Pull request ignores Process instruction data in XML for decoding the data.

Commits
-------

0c741f5 [Serializer] [XML] Ignore Process Instruction
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.