Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cleanup asQBXML() & some undefined constants. #321

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

amorsent
Copy link

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

Doc block lists incorrect method arguments from an older version of the method.
…oks_QBXML_Object.

Why Redundant?
These copies all simply invoke the _cleanup() method and delegate back to QuickBooks_QBXML_Object::asQBXML().
The _cleanup() method is already called by the parent.


Also note: Many of these methods are also using incorrect function arguments.

$todo_for_empty_elements was passed into the $version argument of the parent method.
The default value QUICKBOOKS_OBJECT_XML_DROP is undefined.
PHP would have interpreted this as the string literal 'QUICKBOOKS_OBJECT_XML_DROP' and thrown a warning.
This hasn't caused issues because that argument is effectively unused.

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

1 participant