Cleanup asQBXML() & some undefined constants. #1
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is essentially the same pull request I have filed to consolibyte/quickbooks-php
consolibyte#321
Which itself is loosely based on this one:
consolibyte#194
I am cross posting it here since consolibyte/quickbooks-php appears to be essentially dead, and your fork appears to be the most active.
--- Original pull request description ---
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 consolibyte#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:
consolibyte#15
consolibyte#126
consolibyte#127
consolibyte#185
consolibyte#194
consolibyte#260