Skip to content

Various SimpleXML bugfixes.#955

Closed
danslo wants to merge 13 commits intofacebook:masterfrom
danslo:simplexml-review
Closed

Various SimpleXML bugfixes.#955
danslo wants to merge 13 commits intofacebook:masterfrom
danslo:simplexml-review

Conversation

@danslo
Copy link
Copy Markdown

@danslo danslo commented Aug 13, 2013

Alright, let's try this again!

I've left out the commit that breaks the zend test. All commits below should include a test themselves.

Let me know if I need to change anything.

Daniel Sloof added 13 commits August 13, 2013 14:31
This fixes several things:
- root nodes without children have to return false.
- some_node->children() has to have children to be true.
- child nodes without children are always true!
This already happens using simplexml_load_string, but it must also
happen when directly instantiating and specifying the XML through
constructor.
Perhaps this is not the best implementation. But at least it will allow
us to access cdata.
Most public attributes can be literally copied to the cloned node.
For attributes and children, they have to be regenerated.
- When using specifying a value to addChild, SimpleXML does not display
it when the node is casted to a string, see the following test:
  hphp/test/slow/simple_xml/1641.php

- However, when you cast the return value of this call to string, it
does show a return value, see the following test:
  hphp/test/slow/simple_xml/1656.php (line 13)

- Additionally, this fixes "string nesting" because m_children needed to
be merged with the newly created string element due to the way strings
work in this extension.

Another test was added combining all these conditions.
Just like properties, when dealing with integer based lookups, we need
to iterate ourselves and pick the appropriate child.
SimpleXML will implicitly convert string nodes to actual strings when
the parent node is casted to an array. It will only do this for top
level nodes. A test has been provided.
Once you cast a root element to an array, PHP will consider top level
child nodes as new 'root nodes', these nodes behave differently to child
nodes when it comes to boolean casting.
xmlIsBlankNode has to be called regardless of being a child or parent
text node.
This allows the user to access a node's attribute through
attributes()->some_attribute.

This does not fix the fact that SimpleXML considers attributes to be
actual SimpleXML nodes, rather than just strings. In practice this
should not matter. Such a fix would deserve its own commit.
@scannell
Copy link
Copy Markdown
Contributor

I want to do one final check on this which should happen sometime next week. It looks good so far though.

@danslo
Copy link
Copy Markdown
Author

danslo commented Aug 17, 2013

Thanks for the update!

@brusch
Copy link
Copy Markdown

brusch commented Aug 18, 2013

Works perfectly for me, no more issues with SimpleXML -> thanks danslo!

@ghost ghost assigned scannell Aug 23, 2013
@sgolemon sgolemon closed this in 7a8c847 Aug 23, 2013
@danslo danslo deleted the simplexml-review branch August 24, 2013 01:58
@scannell
Copy link
Copy Markdown
Contributor

@danslo, we hit a problem with this, see https://gist.github.com/scannell/28a6319416738068fdcd. Is this something you can take a look at soon?

@scannell
Copy link
Copy Markdown
Contributor

@danslo: I reverted the specific part of this diff and added a new test in a diff that should hit Github soon. Please rename the two tests I turned off that affect Magento back to .php from .php.disabled and take a look -- the new test I added in that diff will indicate if this broke again.

@danslo
Copy link
Copy Markdown
Author

danslo commented Sep 25, 2013

I think I've decided to port over php's SimpleXML. It might take a while (think weeks) but I'll come up with a cool PR. I'll make sure existing tests pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants