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

4.x - Add route context helper #2759

Merged
merged 1 commit into from Jul 19, 2019
Merged

4.x - Add route context helper #2759

merged 1 commit into from Jul 19, 2019

Conversation

shadowhand
Copy link
Contributor

@shadowhand shadowhand commented Jul 19, 2019

Creates a new RouteContext class that can be used to gain access to the current request route, route parser, and routing results.

Fixes #2758
Fixes #2761

@coveralls
Copy link

coveralls commented Jul 19, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling 42717a2 on shadowhand:feature/route-context into c4bdfa0 on slimphp:4.x.

@adriansuter
Copy link
Contributor

Looks good, althoug I have to check in more depth.

Could you please add the slim file header comment to the two new files?

@shadowhand
Copy link
Contributor Author

@adriansuter

@adriansuter
Copy link
Contributor

In RouteContex::fromRequest(), should we check if the three calls to
$serverRequest->getAttribute() did not return null?

Slim/App.php Outdated Show resolved Hide resolved
Slim/App.php Outdated Show resolved Hide resolved
@l0gicgate l0gicgate added this to the 4.0.0 milestone Jul 19, 2019
@shadowhand
Copy link
Contributor Author

@adriansuter and do what? Throw an exception? PHP will already throw a TypeError if the values are null when trying to construct the RouteContext.

@l0gicgate
Copy link
Member

@shadowhand we need to throw an exception to notify the user that routing hasn't been performed if those attributes are null. Probably throw a RuntimeException

@adriansuter
Copy link
Contributor

adriansuter commented Jul 19, 2019

Or an InvalidArgumentException as actually the given argument does not have sufficient information.

But then, a call to this static method has to be wrapped into a try-catch block. Probably it is better to throw a RuntimeException.

@shadowhand
Copy link
Contributor Author

@l0gicgate @adriansuter updated with your requested changes.

Copy link
Contributor

@adriansuter adriansuter left a comment

Choose a reason for hiding this comment

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

Some imports can be removed.

Slim/App.php Outdated Show resolved Hide resolved
Slim/App.php Outdated Show resolved Hide resolved
Copy link
Member

@l0gicgate l0gicgate 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 to me @adriansuter can you review one last time before I merge please?

@adriansuter
Copy link
Contributor

Thanks - everything is fine.

@adriansuter
Copy link
Contributor

We should not forget to update the docs about this new feature.

@l0gicgate
Copy link
Member

@adriansuter absolutely

@l0gicgate l0gicgate merged commit 04be469 into slimphp:4.x Jul 19, 2019
@l0gicgate l0gicgate changed the title Add route context helper 4.x - Add route context helper Jul 19, 2019
@shadowhand shadowhand deleted the feature/route-context branch July 19, 2019 18:23
@l0gicgate l0gicgate mentioned this pull request Aug 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants