odan / slim4-skeleton

A Slim 4 Skeleton
https://odan.github.io/slim4-skeleton/
MIT License
439 stars 80 forks source link

Inject RouteParser to UrlGenerator constructor #34

Closed ybelenko closed 3 years ago

ybelenko commented 3 years ago

Small refactoring.

It seems easier to expect RouteParserInterface as constructor argument than retrieve it with RouteContext::fromRequest every time.

I'm not sure about performance, but all urls has been computed correctly.

I guess fullUrlFor will throw exception when $this->request is null anyway.

sonarcloud[bot] commented 3 years ago

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

odan commented 3 years ago

The request and the RouteParser is context specific and the container should be "decoupled" from the request/response objects. Moving the RouteParserInterface to the constructor (for autowiring) would mean that the dependency injection container needs to know about the request object. This is what I try to prevent as much as possible.

ybelenko commented 3 years ago

Hmm, why do you keep RouteParser injection in container then? https://github.com/odan/slim4-skeleton/blob/9154094f65a53464903764380e1dae97951b1918/config/container.php#L53-L56

ybelenko commented 3 years ago

Let me explain my use case a bit. I would like to catch common exceptions in DefaultErrorHandler. I need to grab data from remote API almost in all route Actions and when my API token expired it throws GuzzleClientException. To refresh API token I need to force user to login again, which means I need to make redirect to login route.

I tried to create UrlGenerator right there https://github.com/odan/slim4-skeleton/blob/9154094f65a53464903764380e1dae97951b1918/src/Handler/DefaultErrorHandler.php#L67-L72 I passed $request variable down to $generator = new UrlGenerator($request) and received RuntimeException('Cannot create RouteContext before routing has been completed').

RuntimeException disappeared when I changed UrlGenerator constructor to implementation from this PR .

If you have better suggestion how to make redirect in DefaultErrorHandler I would love implement it.

odan commented 3 years ago

The UrlGenerator only works correct in combination with the UrlGeneratorMiddleware.

Have you added this middleware to your stack?

use App\Middleware\UrlGeneratorMiddleware;

// ...

$app->add(UrlGeneratorMiddleware::class);`

RuntimeException('Cannot create RouteContext before routing has been completed')

This happens when your request object is not passed through the Slim RoutingMiddleware. Do you create your own request object manually?

ybelenko commented 3 years ago

The UrlGenerator only works correct in combination with the UrlGeneratorMiddleware.

Have you added this middleware to your stack?

Yes, just checked. Middleware config hasn't been changed. Same as in this repo.

use App\Middleware\UrlGeneratorMiddleware;

// ...

$app->add(UrlGeneratorMiddleware::class);`

RuntimeException('Cannot create RouteContext before routing has been completed')

This happens when your request object is not passed through the Slim RoutingMiddleware. Do you create your own request object manually?

No, I pass $request instance provided as argument to __invoke() method of DefaultErrorHandler.