Skip to content

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Jul 6, 2022

This PR changes the hierarchy of Exceptions for Shield as follows:

  • BaseException
    • LogicException
      • InvalidArgumentException
    • RuntimeException
      • AuthenticationException
      • AuthorizationException

Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Your desired hierarchy would work if your PR to the framework for exception interfaces goes through. To that end we should consider making these exception classes @internal or final or both (which is probably a good idea anyway).

@kenjis kenjis force-pushed the fix-exception-structure branch from 5a21fa2 to 63ca0af Compare July 11, 2022 07:32
@kenjis
Copy link
Member Author

kenjis commented Jul 11, 2022

Shield\Exceptions\HTTPException was not needed.
Since CodeIgniter\HTTPExceptionsExceptions\HTTPException was used in AuthenticationException,
I thought we needed an exception to wrap it. However, a closer look at the code showed that it only took the caught CodeIgniter\HTTPExceptions\HTTPException and converted it into a new exception.

So I removed Shield\Exceptions\HTTPException.

But I found AuthenticationException has some LogicException.
Since Config data may come from outside at runtime, these are considered RuntimeException.

@kenjis kenjis force-pushed the fix-exception-structure branch 2 times, most recently from 1898dc6 to 3eafffe Compare July 11, 2022 08:24
@kenjis kenjis force-pushed the fix-exception-structure branch from 3eafffe to d8342dd Compare July 11, 2022 08:26
@MGatner
Copy link
Member

MGatner commented Jul 11, 2022

Just out of curiosity what is the advantage of implementing our own version if InvalidArgumentException? Logic errors by definition is code that needs fixing so shouldn't be caught, which makes me think that adding them to BaseException means someone might accidentally suppress a logic error by catching all BaseException.

@kenjis kenjis merged commit dadd7b3 into codeigniter4:develop Jul 15, 2022
@kenjis kenjis deleted the fix-exception-structure branch July 15, 2022 07:58
@kenjis
Copy link
Member Author

kenjis commented Jul 15, 2022

The reason for placing all exceptions under BaseException is to make it easier to know where the exception came from.

It is the developer's responsibility to decide how to handle the exception.

If a dev catches BaseException, it seems they want to catch all exceptions by Shield.

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.

2 participants