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

Configurable exception on missing resource #36

Open
wants to merge 2 commits into
base: master
from

Conversation

@Einenlum
Copy link

commented Mar 28, 2017

Improve #33 by adding a throw key in the on-missing parameter.

@Einenlum Einenlum added ToDo Ready to review and removed ToDo labels Mar 28, 2017

@Einenlum Einenlum force-pushed the feature/configurable-missing branch 5 times, most recently from 707cf8a to 2ba6ac4 Mar 28, 2017

Einenlum

@Einenlum Einenlum force-pushed the feature/configurable-missing branch from 2ba6ac4 to f18b3c3 Mar 28, 2017

@@ -94,6 +105,21 @@ public function addParameterCaster(ParameterCaster $parameterCaster)
return $this;
}
private function throwMissingException(array $configuration, $resourceKey)
{
if (!in_array($configuration['throw'], array_keys($this->exceptions))) {

This comment has been minimized.

Copy link
@NiR-

NiR- Mar 28, 2017

Member

$this->exceptions is not initialized.

method: findByCountryAndCityAndActivity
arguments: [$countryId, $citySlug, "School"]
on-missing:
throw: 401

This comment has been minimized.

Copy link
@NiR-

NiR- Mar 28, 2017

Member

Would be cool to directly pass an exception FQCN

This comment has been minimized.

Copy link
@Einenlum

Einenlum Sep 22, 2017

Author

@NiR- Sounds interesting. Would we be able to also configure the message exception, then?

This comment has been minimized.

Copy link
@NiR-

NiR- Sep 22, 2017

Member

If we would support exception message, we would have to support other parameters I guess. I think it would be better to use a message defined by this component and indicating which resource resolution failed.

@NiR-

This comment has been minimized.

Copy link
Member

commented Sep 22, 2017

Since php 5.6 has been dropped by this component, I've some doubt about this PR, and more generally with the current behavior when a resource isn't resolved. Following best practices, I would implement find methods with a return type, thus it can return only that type, and not null. If it's not found I would throw an exception. Currently, no exceptions are caught during resource resolution.

EDIT: Beside exceptions not caught, there's another problem: how to distinguish "ResourceNotFound" exceptions and legitimate exceptions? Do we need to force the use of a specific base class/interface? I guess it would be quite ugly :/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.