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

Optionally handle subclasses of exceptions in custom error handler #2862

Merged
merged 2 commits into from Dec 10, 2019

Conversation

@nickdnk
Copy link
Contributor

nickdnk commented Oct 8, 2019

Hello guys

I thought this could be a nice addition, so a particular app can decide to handle all subclasses of an exception with one handler. This is nice way to keep things separated.

I added this as an option that defaults to false so this should not be a breaking change, but I'm not 100% sure since $handlers is protected and not private.

What do you think?

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 8, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling 7ea0a3d on nickdnk:4.x into ea75cf4 on slimphp:4.x.

@nickdnk nickdnk force-pushed the nickdnk:4.x branch from 8cbcaa6 to 1ff7d92 Oct 8, 2019
@nickdnk

This comment has been minimized.

Copy link
Contributor Author

nickdnk commented Oct 8, 2019

Force pushed for typo.

@nickdnk

This comment has been minimized.

Copy link
Contributor Author

nickdnk commented Oct 8, 2019

We could mitigate the BC by doing this instead:

if (isset($this->handlers[$type])) {
    return $this->callableResolver->resolve(
        is_array($this->handlers[$type]) ? $this->handlers[$type]['handler'] : $this->handlers[$type]
    );
} else {
    foreach ($this->handlers as $class => $handler) {
        if (is_array($handler) && $handler['handle_subclass'] === true) {
            if (is_subclass_of($type, $class)) {
                return $this->callableResolver->resolve($handler['handler']);
            }
        }
    }
}

Then even if a someone has subclassed ErrorMiddleware and overriden $handlers to contain callables while not overriding the setErrorHandler method, it still wouldn't break. Why someone would do that I don't know though.

@l0gicgate

This comment has been minimized.

Copy link
Contributor

l0gicgate commented Oct 12, 2019

@adriansuter I think this would be a good addition and there's no BC breaks. What do you think about it?

@l0gicgate l0gicgate added the Slim 4 label Oct 12, 2019
@nickdnk

This comment has been minimized.

Copy link
Contributor Author

nickdnk commented Oct 12, 2019

Is there a reason that $handlers is protected and not private ? Given how close this variable is tied to the setErrorHandler method, should it be visible to a subclass?

@l0gicgate

This comment has been minimized.

Copy link
Contributor

l0gicgate commented Nov 8, 2019

Is there a reason that $handlers is protected and not private ? Given how close this variable is tied to the setErrorHandler method, should it be visible to a subclass?

In case one wants to extend ErrorMiddleware we left it as protected.

@l0gicgate l0gicgate added this to the 4.4.0 milestone Nov 8, 2019
@l0gicgate l0gicgate requested a review from adriansuter Nov 8, 2019
Copy link
Contributor

adriansuter left a comment

That is a good idea.

Another solution would be to add a new protected property subClassHandlers and then just write

    public function setErrorHandler(string $type, $handler, bool $handleSubclasses = false): self
    {
        if ($handleSubclasses) {
            $this->subClassHandlers[$type] = $handler;
        } else {
            $this->handlers[$type] = $handler;
        }

        return $this;
    }

That way, we do not change the data logic of the handlers property which is expected to be a plain array of handlers.

Of course we would have to change getErrorHandler() too.

    public function getErrorHandler(string $type)
    {
        if (isset($this->handlers[$type])) {
            return $this->callableResolver->resolve($this->handlers[$type]);
        } elseif (isset($this->subClassHandlers[$type])) {
            return $this->callableResolver->resolve($this->subClassHandlers[$type]);
        } else {
            foreach ($this->subClassHandlers as $class => $handler) {
                if (is_subclass_of($type, $class)) {
                    return $this->callableResolver->resolve($handler);
                }
            }
        }

        return $this->getDefaultErrorHandler();
    }

That way, we would also get rid of the double if-statements in the foreach loop.

What do you mean, @l0gicgate and @nickdnk ? If you want to leave it as requested in this PR, then I am fine too.

@nickdnk

This comment has been minimized.

Copy link
Contributor Author

nickdnk commented Nov 8, 2019

I think yours is a good workaround, @adriansuter.

@l0gicgate

This comment has been minimized.

Copy link
Contributor

l0gicgate commented Nov 18, 2019

@adriansuter I like your solution. We should proceed with that.

@nickdnk nickdnk force-pushed the nickdnk:4.x branch from d1b6664 to 93671b7 Nov 19, 2019
@nickdnk

This comment has been minimized.

Copy link
Contributor Author

nickdnk commented Nov 19, 2019

Updated the PR.

Also, I added phpdocs for the handler arrays based on the value of $defaultErrorHandler - but I'm not entirely sure that you can annotate them as ErrorHandlerInterface since they're functions? Edit: Aha, so if you pass that interface in then it will behave as a function when __invoke is called. Got it.

@l0gicgate

This comment has been minimized.

Copy link
Contributor

l0gicgate commented Nov 20, 2019

Need to write tests as well for the subclass case. Are you able to do that?

@nickdnk

This comment has been minimized.

Copy link
Contributor Author

nickdnk commented Nov 20, 2019

I did. I don't know why it claims it wasn't covered. There is a test for both with and without subclass use.

@adriansuter

This comment has been minimized.

Copy link
Contributor

adriansuter commented Nov 20, 2019

Did you add a test case for the case you set an error handler as subclass handler, but the exception thrown is not a subclass but exactly that class?

} elseif (isset($this->subClassHandlers[$type])) {

@nickdnk nickdnk force-pushed the nickdnk:4.x branch from fcb3893 to f515e82 Nov 20, 2019
@nickdnk

This comment has been minimized.

Copy link
Contributor Author

nickdnk commented Nov 20, 2019

I think that did it.

tests/Middleware/ErrorMiddlewareTest.php Outdated Show resolved Hide resolved
tests/Middleware/ErrorMiddlewareTest.php Outdated Show resolved Hide resolved
@nickdnk nickdnk force-pushed the nickdnk:4.x branch from 0980356 to 6c0528a Dec 1, 2019
…ndlers

Implemented subclass handler array to avoid breaking changes
Cleaned up test syntax a bit
@nickdnk nickdnk force-pushed the nickdnk:4.x branch from 6c0528a to 70c33c6 Dec 1, 2019
@nickdnk nickdnk requested a review from l0gicgate Dec 1, 2019
@l0gicgate l0gicgate merged commit 941828c into slimphp:4.x Dec 10, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 100.0%
Details
@l0gicgate l0gicgate mentioned this pull request Jan 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.