quickbooks-php icon indicating copy to clipboard operation
quickbooks-php copied to clipboard

Cleanup asQBXML() & some undefined constants.

Open amorsent opened this issue 1 year ago • 0 comments

Overview:

  • Fix QuickBooks_QBXML_Object::asQBXML() doc block to reference correct arguments
  • Drop unused $root argument.
  • Remove redundant (and incorrect) asQBXML() implementations from subclasses (more explanation below)

Note: This is a simplified and more focused version of #194. My version stays focused on the asQBXML() method for now, and I've kept the commits clean and hopefully easy to follow. However, @cmancone is correct that _cleanup() and asArray() have similar issues.

Why are the subclassed implementations Redundant? These copies all simply invoke the _cleanup() method and delegate back to the central QuickBooks_QBXML_Object::asQBXML() which already calls the _cleanup() method. _Also note: in most cases cleanup() is an empty function anyway.

Many of these methods are also using incorrect function arguments and undefined constants. $todo_for_empty_elements was passed into the $version argument of the parent method. The default value QUICKBOOKS_OBJECT_XML_DROP is undefined and PHP interprets this as the string literal 'QUICKBOOKS_OBJECT_XML_DROP'. This hasn't caused issues because that argument is effectively unused by the parent method. (There is an if block that checks it, but it does nothing)

$indent was being passed into $locale in the parent method. The default value "\t" is technically one character and the parent method ignores anything that isn't exactly 2 characters.

Effect of dropping the method overrides:

  • The base implementation will be called instead.
  • _cleanup() will only be called once.
  • $version will default to null instead of 'QUICKBOOKS_OBJECT_XML_DROP' (it is ignored anyway, so no change)
  • $locale will default to null instead of "\t" (no change in outcome)
  • If any caller passes other values, they would still pass through the same with no change in outcome.

Related: There are several other issues and pull requests related undefined constants. This pull request would at least partially address many of them.

Here's a few: #15 #126 #127 #185 #194 #260

amorsent avatar Feb 24 '23 21:02 amorsent