Package home | Report new bug | New search | Development Roadmap Status: Open | Feedback | All | Closed Since Version 0.2.1dev1

Bug #13951 XML data with only one "row" can't be parsed on PHP 4
Submitted: 2008-05-20 18:55 UTC
From: wiesemann Assigned: olivierg
Status: Closed Package: Structures_DataGrid_DataSource_XML (version CVS)
PHP Version: 5.2.6 OS: Irrelevant
Roadmaps: (Not assigned)    

 [2008-05-20 18:55 UTC] wiesemann (Mark Wiesemann)
Description: ------------ On PHP 4 XML data with only one row cannot be parsed. A solution would be use the following line of code: $unserializer->setOption('forceEnum', array('row')); 'row' is the tag name of the second-level, e.g.: <data> <row>...</row> <row>...</row> </data> I'm not sure whether we could (and should) auto-detect this tag name or whether we should provide a new option for this tag name, so that our users could pass the name to the class in case they expect data with only one row. Comments please?


 [2008-05-21 11:42 UTC] olivierg (Olivier Guilyardi)
I think the xpath option should be sufficient for many needs, there's no need to add a new option for this. So IMO, we should use this forceEnum option. Now regarding our current way to consider that second level tags are rows, I think this has some limitations, and that's what my new testTransversalXpath() test tries to show. A solution could be to always use the first level tags as rows, and a default xpath of "./*", which means "all children of the current (root in our case) node". See: $xml = '<a><b>1</b><b>2</b></a>'; $xml = simplexml_load_string($xml); $tags = $xml->xpath('./*'); foreach ($tags as $item) { echo $item->asXML() . "\n"; } results in: <b>1</b> <b>2</b> This way the xpath option would offer great flexibility, but it would slightly break BC, for those who already use the xpath option. For example: "/result/records" would have to be changed to "/result/records/record" or "/result/records/*" I'm sorry this is getting a bit out of the scope of your report, but I think you're touching a wider issue.
 [2008-05-21 11:53 UTC] olivierg (Olivier Guilyardi)
Actually, the default xpath could even be simplified to "*" instead of "./*"
 [2008-05-21 13:40 UTC] wiesemann (Mark Wiesemann)
Your proposal makes sense, yes. This would be worth breaking BC for some users. But how you do want to detect the tag name for the forceEnum option? Via a regular expression?
 [2008-05-21 15:28 UTC] olivierg (Olivier Guilyardi)
Well, you could use xpath, assuming its default is "*": $set = simplexml_load_string($xml)->xpath($this->_options['xpath']); echo $set[0]->getName(); will print "row" in your example. This is SimpleXML but should be as easy with XML_XPath. Now this is going to cause another BC break: until now we used XML_XPath only when xpath was set. But with this change it would be loaded every time, and since it depends on PHP4 domxml (which is often missing in my experience) that may break a few users' scripts. The same problem happens with my idea of using a default xpath of "*". But this could be solved by checking to see if its equal to "*" and keeping the default behaviour if that's the case, otherwise run the xpath engine. So, to avoid xpath if possible, maybe that you don't need the "forceEnum" option: why not checking if $data['row'][0] exists, and if it doesn't use $data['row'] as the only row? There's no risk of collision with a tag name, because in XML tag names can't only consist in numbers. Last point, to avoid the BC break introduced with the new default xpath of "*", we could obsolete (but keep hidden) the xpath option, and add a new rowsPath. Setting the xpath option would override rowsPath with a value of "$xpath/*". Internally, the only path we would use is the value of rowsPath.
 [2008-05-21 19:13 UTC] wiesemann (Mark Wiesemann)
About your first paragraph: With SimpleXML this would be very easy, but this bug is only about PHP 4. Apart from this, I really like your proposal. Please go ahead with the proposed changes (new option, 0 index check). This sounds like a working solution without too much pain for the existing users.
 [2008-05-23 10:55 UTC] olivierg (Olivier Guilyardi)
Done + other goodies :) Do the tests pass on your side?
 [2008-05-23 18:44 UTC] wiesemann (Mark Wiesemann)
Great job, the tests pass on both PHP 4 and PHP 5 here. Thanks for also fixing the "redundant fields" problem. I noticed this while testing my last changes, and would have fixed it soon. Multisorting is also a nice addition. The "FIXME" comment in the XML driver is now obsolete, isn't it? (I'll answer the open email within the next days ...)
 [2008-05-23 19:32 UTC] olivierg (Olivier Guilyardi)
Hey, well... the FIXME's gone, trust me. I completely rewrote the driver. I was pretty unhappy with the result of Atom parsing (see bind-atom.php), and testFieldAndLabelAttributes() was wrong. Sorry about that, I made you fix it for nothing. So, when trying to fix these issues, I realized the maintenance of this dual head driver was becoming a nightmare. This is why I've rewritten it, using thin DOM wrappers. I hope you'll like it, it's quite a lot of work... There's a few tests that still fails, but there's something beautiful about it: it fails in the exact same way under both PHP4 and PHP5 :)
 [2008-05-24 12:25 UTC] wiesemann (Mark Wiesemann)
As long as you make the tests succeeding again, I'm fine with this refactoring. Maintainance should really get easier with only one common piece of code.
 [2008-05-26 15:06 UTC] olivierg (Olivier Guilyardi)
Done, except for testDeeperNesting(). This feature conflicts with testNestedHTML(), which is needed for example for Atom parsing. Do you have a real life example for which this feature would be useful?
 [2008-05-26 18:14 UTC] wiesemann (Mark Wiesemann)
No, I don't have an example. I just noted that the PHP 5 part wasn't able to parse such deeper nested XML tags, while the PHP 4 part did (the prepending of "attributes" in the example of the other bug made me aware of this difference). If we drop the support now, people could complain about this BC break, but on the other hand, nobody complained about the fact that this isn't possible with the current release (available since about half a year). => I'm fine with dropping the test.
 [2008-05-28 10:58 UTC] olivierg (Olivier Guilyardi)
Agreed. I removed the test and updated dependencies. I'm closing this bug.