Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TASK: Base exception on \RuntimeException instead of \DomainException for ESCR core #5399

Draft
wants to merge 1 commit into
base: 9.0
Choose a base branch
from

Conversation

mhsdesign
Copy link
Member

@mhsdesign mhsdesign commented Dec 9, 2024

Initially i adapted the style to use DomainExceptions too which is part of this RFC: https://discuss.neos.io/t/rfc-updated-default-code-style/5836#exceptions-5 but then @bwaidelich pointed out that this is not correct (todo why?) and that we should not base exceptions on that.

There are already 50 cases where use throw \RuntimeException and WorkspaceIsNotEmptyException, PartialWorkspaceRebaseFailed, WorkspaceRebaseFailed

As i was tempted to change it for the WorkspaceDoesNotExistException which will be thrown very likely via #5334 (comment) i might as well do it once and for all for all cases before we have to do it later (its fairly simple string replace)

We just have to agree on a common pattern here.

Disclaimer: Im not really that invested in whatever we agree on - as long as everybody is happy and its consistent. Maybe we agree that is currently okay as well?

Todo:

  • when to use class AccessDenied extends \Exception instead of \RuntimeException?
  • check other new escr like packages

Upgrade instructions

Review instructions

Checklist

  • Code follows the PSR-2 coding style
  • Tests have been created, run and adjusted as needed
  • The PR is created against the lowest maintained branch
  • Reviewer - PR Title is brief but complete and starts with FEATURE|TASK|BUGFIX
  • Reviewer - The first section explains the change briefly for change-logs
  • Reviewer - Breaking Changes are marked with !!! and have upgrade-instructions

@kitsunet
Copy link
Member

kitsunet commented Dec 9, 2024

I'd say this is a case by case decision, so far I am mostly for keeping DomainException. Unfortunately the documentation is rather unclear on the SPL Exceptions but:

Exception thrown if a value does not adhere to a defined valid data domain.

Ok so our domain is the CR obviously.

Someone provided this example code which I like:

function divide($divident, $divisor) {
    if(!is_numeric($divident) || !is_numeric($divisor)) {
        throw new InvalidArgumentException("Function accepts only numeric values");
    }
    if($divisor == 0) {
        throw new DomainException("Divisor must not be zero");
    }
    return $divident / $divisor;
}

I'd say the errors here are not specificly runtime problems but mostly domain errors....

Couple of them like NodeAggregateIsAlreadyEnabled could be a LogicError, some others could be InvalidArgumentException, but RuntimeException I don't find ideal.

@bwaidelich
Copy link
Member

for the record: I never claimed that the DomainException is not correct per se. But we IMO use it incorrectly in some parts.
The term "domain" is overloaded – there's even a bug report about the lack of decisive description in the php docs.
I like the take of Marcus Boerger (author of the SPL):

Domain means data domain here. That is a DomainException shall be through whenever a value does not adhere to a defined valid data domain. Examples:

  • 0 is not a in the domain for division.
  • Foo is not in the domain for weekdays.

The first is different from out of range and alike, but you could use InvalidParameter in case it is actually a parameter to the function that performs the division. If it is a value calculated inside the function prior to executing the division and then a pre-conditon check throws instead of executing the division, then it becomes a DomainException.

The second could be seen as an OutOfRangeException.

@kitsunet
Copy link
Member

Yep I think we agree here, but I definitely wouldn't just blanket change everything to RuntimeException, I think that is a disservice and less accurate than the current state.

@mhsdesign mhsdesign marked this pull request as draft December 12, 2024 18:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants