[
https://issues.apache.org/jira/browse/SHINDIG-642?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Chris Chabot resolved SHINDIG-642.
----------------------------------
Resolution: Fixed
Assignee: Chris Chabot
There were a some issues with the patch:
- Patch was made without an identifiable root.. better to do a svn diff from
shindig/php or shindig/, that way i dont have to hunt down file names :)
- Patch didn't apply cleanly, had to apply it manually
- $xmlErrors= $this->display_xml_error($error, $xmlString); ... the name
"errors" implies you want to store them all, but you over-write it every time?
Better to do $xmlErrors .= $this->display_xml_error($error, $xmlString); right?
- libxml_use_internal_errors(true); in the global scope ... either this should
be in a place where it's easy to find (index.php?) or wrapped in the function
... otherwise doing this on file inclusion could lead to some confusion..
- code formatting wasn't really consistent with our internal format (spaces
after a , and before & after ='s please)
- $xmlString = explode("\n", $xml); and function
display_xml_error($error,$xmlString), but your not using $xmlString anywhere?
shouldn't be there then!
- public function display_xml_error($error,$xmlString) ... when only
GadgetSpecParser uses the function, why make it public? it's not a documented
SPI, and if you meant for it to be re-usable, the GadgetSpecParser class isn't
the right place to put generic xml parsing functionality ... also the function
name isn't quite consistent with our naming scheme (displayXmlError would be
more fitting), it would appear we're a victim of cut and paste coding ? :)
- I'm also quite hesitent to include html markup (<br>, <b>, etc) in generic
functionality ... in the metadata rpc interface for instance html markup has no
place, but would be included now anyhow ... layout code shouldn't be hidden
deeply in the spec parser, but in the displaying of the error!
- $return .= "Warning $error->code", this should be "Warning {$error->code}",
else it'll fail on many php instalations (and generate notices in other
situations), always a good idea to develop using error_reporting =
E_ALL|E_STRICT (php.ini) !
- $return .= '...' before declaring $return = '' gives a error when E_STRICT is
set
Thanks for the patch though, and it's good to have this functionality in
php-shindig, however I do hope the next patch will be in a better shape :)
> Need more explanatory Xml errors for debugging in GadgetSpec Parsing.
> ---------------------------------------------------------------------
>
> Key: SHINDIG-642
> URL: https://issues.apache.org/jira/browse/SHINDIG-642
> Project: Shindig
> Issue Type: Improvement
> Components: Gadget Rendering Server (PHP)
> Reporter: impetus technologies
> Assignee: Chris Chabot
> Priority: Minor
> Attachments: GadgetSpecParser.patch
>
>
> Currently Shindig gives only "Invalid Xml document" message with debug back
> trace. Patch is attached.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
