Separate error codes for "invalid_return"

[rolandseuhs]

Hi,

I currently have a very hard-to-debug issue where I get the "invalid_return" error, which is generated in parseResponse(). In my case it does make a difference where the error was generated.

Therefore I would like to split up the error that is currently "invalid_return".

would you accept a patch where I would separate this error-code into three separate error codes?

Would you be fine with:

"return_invalid_xml" for the first error check (you have put "// first error check: xml not well formed" before this check)
"return_not_xmlrpc_compliant" for the second error check (you have put "// second error check: xml well formed but not xml-rpc compliant" before this check
"return_parse_error" for the third check (you have put "// third error check: parsing of the response has somehow gone boink." before this check

This way you would get a little more information when something goes wrong, also I could send a useful error-message to the partner on the other side, which may help him to fix the issue.

[gggeek]

Hello. This makes sense, but it would be a BC break for all other users of the lib who might be checking the current error code.
Maybe we could find a compromise solution which achieves both things at the same time?

[gggeek]

ps: about giving back meaningful errors to the caller: I think that the best value for money would be in identifying the line number in the original xml, but I am not sure that the current parser is able to do that...
(edit: in the case of malformed xml we do ask for the error line to the underlying libxml implementation, but in my experience those errors range from cryptic to outright sidetracking)

[rolandseuhs]

Which case is the most common one?

Maybe keep "invalid_return" for the most often encountered error and create 2 new codes for the not-so-often encountered errors?

[gggeek]

I was thinking along the line of defining 3 consts with the same value, and allowing subclasses to redefine their value as being separate, or some other weird trick tbh... Or maybe addding a 2nd error (sub)code, but that seems quite overkill

[gggeek]

Another idea: add a separate method (or switch) which calls the same parsing routines in "debug mode", which would generate more info...

[rolandseuhs]

maybe something like:

if ($backwards_compatible_mode && ($errNo == <new code> || $errNo == <second new code>))
{   
    $errNo = 2;
}

[rolandseuhs]

maybe something like:

if ($backwards_compatible_mode && ($errNo == [new code] || $errNo == [other new code]))
{ 
    $errNo = 2;
}

[gggeek]

Back after having checked the code.

I think there is a better solution than what i said in my previous suggestions.

Atm the error code 2 ('invalid_return') is used to cover different cases of invalid responses, but the detailed reason for the error is stored as part of the error string, which you can get from the Response object via a call to $response->faultString().
I think that that is already good enough for you to send to the partner - in fact much better than the error code.

If you want to transform the current error strings into something else, a not-super-clean-but-working-nonetheless solution would be to simply match string patterns, eg:

if ($response->faultCode() === 2) {
    $error = $response->faultString();
    if ($error === \PhpXmlRpc::$xmlrpcstr['invalid_return']) {
        // only possible case: line 365 in Request.php - internal parsing error
    } elseif (preg_match('/' . \PhpXmlRpc::$xmlrpcstr['invalid_return'] . ' XML error %d: /', $error) {
        // 'isf' originally 3: invalid or empty xml...
    } else {
        // 'isf' originally 2: xml not compliant with xmlrpc...
    }
}

[gggeek]

ps: might it help if we were to split the Request::parseResponse method call into multiple protected submethods, making it easier to override just one of those in a subclass?

[rolandseuhs]

I'm a proponent of keeping it simple and keeping code for backwards compatibility separate so that it can be removed when it is no longer needed (again to keep it simple in the future).

Therefore I would propose to implement it cleanly and then add a backwards-compatible flag (which can be set to off until the next major release, then set to on, and finally removed in some future major release) so that no bloat is permanently added.

Adding subclasses or string-comparisons against error-messages would make the code much more complicated, harder to understand and most importantly almost impossible to remove in the future.

[gggeek]

I do agree that simplicity and not having to carry around BC hacks are worthy goals.
I still am not convinced about adding a $backwards_compatible_mode flag, though.
About subclasses and string comparison: my proposal was for you to implement those on your side, not to add them to the library. So it would be the simplest solution possible for phpxmlrpc ;-)

Having said that, you did not answer to the question about why the currently available error string is not suitable for your needs.

As for the most elegant way of extending the error code while keeping BC, I think my idea of replacing the current PhpXmlRpc::$xmlrpcerr['invalid_return'] with 3 different new codes, such as fe. PhpXmlRpc::$xmlrpcerr['invalid_xml'], PhpXmlRpc::$xmlrpcerr['xml_not_compliant'] and PhpXmlRpc::$xmlrpcerr['xml_parsing_error'] while giving them the same numeric value is the best way forward.
Since those are not constants, it would be easy for you to redefine their value once in your code, and be able to check for them separately.
Then, when the library switches over to its next major version, it will redefine the numeric values too - and your code would still keep working, untouched.
Would that be acceptable?

[gggeek]

Implemented as described above. Will be in next release (4.9.0)