[jira] Resolved: (SHINDIG-642) Need more explanatory Xml errors for debugging in GadgetSpec Parsing.

["Chris Chabot (JIRA)" <ji...@apache.org>]

     [ 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.