Fixed issue with Throwable being passed to createApiProblemFromException

[javabudd]

This PR addresses #102.

An instance of Throwable is being passed into the createApiProblemFromException method, causing an ArgumentException.

I have separated the functions and done minor cleanup (imports, protected/private, .gitignore)

[timiTao]

When this will be in master?

[lrepolho]

Such an important repository as zf-rest don't get anything merged into master since Jul 12 and this fix is available since August and no one gives a f***. This kind of thing is incredibly disappointing, everyday is a struggle to work with newest versions of Zend tools.

[weierophinney]

@LeandroCR I understand your frustration. However, please be aware that we have over 100 repositories to maintain, and less than 20 maintainers capable of reviewing and merging requests. Prioritization is, understandably, a difficult affair.

Venting publicly such as you have done is dismissive of the amount of effort made to provide adequate review of all incoming patches, and I request that, in the future, you please be more respective of the various contributors and maintainers in the project.

[weierophinney]

Please remove this; IDE-specific rules should be in your global gitignore file.

[weierophinney]

As noted in comments within this review, I'd argue for simply removing the typehint from createApiProblemFromException() (potentially renaming it to indicate Throwable is an appropriate argument), and calling that method from both catch blocks.

Additionally, we need unit tests to validate the functionality, and demonstrate the behavior being addressed. These should be setup to skip on non-PHP 7 versions.

[weierophinney]

There's no reason to split this functionality into two separate methods, particularly as, with private visibility, the methods are clearly internal. Combine them into one, and remove the typehint.

[weierophinney]

If the createApiProblemFrom*() methods are merged, technically you wouldn't need to separate this into a new method. However, I think it makes the other methods more readable, so I'm fine if this stays as submitted.

[javabudd]

I have combined the methods and done some minor docblock fixes.

Additionally, we need unit tests to validate the functionality, and demonstrate the behavior being addressed. These should be setup to skip on non-PHP 7 versions.

I can take a stab at unit tests on a separate branch.

[weierophinney]

I can take a stab at unit tests on a separate branch.

Please include them in this branch, and just push the additions when ready.

[weierophinney]

I've added some more review points. In summary:

• Keep the original method names
• Keep the original visibility
• Update the argument only to remove the typehint, and then detail in the docblock the types allowed when calling the methods.

Thanks!

[weierophinney]

In reviewing, I realized something: the original was protected, so that it could be called by extending classes. Let's make it protected again, and reinstate the original name (createApiProblemFromException()). Update the docblock to indicate that the $error argument can be either a PHP 7 Throwable or any Exception type.

The argument can drop the typehint safely in a maintenance release, as that's a widening of types, vs. narrowing.

[weierophinney]

Just like the createApiProblem() notes, just re-instate the original method, but drop the typehint from the method signature and update the docblock to indicate what types are allowed; this is necessary to preserve backwards compatibility, and to allow users to re-use these methods in their extending classes.

[weierophinney]

Merged with #105. Thanks, @javabudd !