eloquent / enumeration

An enumeration implementation for PHP.
MIT License
147 stars 8 forks source link

Use a throwable in php doc #29

Closed yannickroger closed 3 years ago

yannickroger commented 3 years ago

The phpdoc throws interfaces, which are not Throwable, so it makes it impossible to catch the exception without catching undocumented exception.

In this MR, I deprecate the interface and replace the usages by the abstract class that can serve as a common type instead.

codecov[bot] commented 3 years ago

Codecov Report

Merging #29 (f4a12d5) into master (9fd9c8b) will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##              master       #29   +/-   ##
===========================================
  Coverage     100.00%   100.00%           
  Complexity        56        56           
===========================================
  Files              6         6           
  Lines            174       174           
===========================================
  Hits             174       174           
Impacted Files Coverage Δ
src/AbstractMultiton.php 100.00% <ø> (ø)
src/AbstractValueMultiton.php 100.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 9fd9c8b...f4a12d5. Read the comment docs.

ezzatron commented 3 years ago

Thanks for the PR!

Can you help me to better understand the issue you're trying to solve? Interfaces can be caught just fine in PHP (live demo):

interface InterfaceA {}
class ExceptionA extends Exception implements InterfaceA {}

function throwException()
{
    throw new ExceptionA();
}

try {
    throwException();
} catch (InterfaceA $error) {
    printf('Caught %s', get_class($error)); // prints "Caught ExceptionA"
}

The decision to use interfaces over abstract base classes for exceptions is a deliberate one, because it doesn't lock every exception that could be thrown into using a particular implementation. See this article for some deeper explanation.

yannickroger commented 3 years ago

You are right it seems I don't have a problem.

The issue I had (a few months ago in a similar situation) was to then pass the Interface to another exception, and static analyzer (like phpstan) would complain that the interface was not Throwable and could not be provided to the constructor of the Exception. I made this MR without actually having the issue this time. But I just tested and it seems to work just fine.

        } catch (UndefinedMemberExceptionInterface $e) {
            throw new \Exception('bad type', 0, $e);
        }

It seems analyzer got smarter (maybe it is checking all the usage of this interface making sure they are throwable).

Sorry for the noise.

ezzatron commented 3 years ago

Ah yep, I think I've seen PHPStan complain about that sort of thing in the past too. No worries, glad it's working!