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

[5.7] Add original parameters to Route #27056

Merged
merged 2 commits into from Jan 5, 2019

Conversation

Projects
None yet
4 participants
@d3jn
Copy link
Contributor

d3jn commented Jan 3, 2019

Recently had a need to retrieve original (on-bound) route's parameters values (similar to original attributes values in Eloquent), such feature was requested for some time too. Here is the issue laravel/ideas#1260 where this feature has been requested/discussed previously.

This PR just adds additional field to Illuminate\Routing\Route to preserve parameters original state - this field is initialized only once when route is bound and has public getters that mimic already existing methods for parameters field.

@GrahamCampbell GrahamCampbell changed the title Add original parameters to Route [5.7] Add original parameters to Route Jan 3, 2019

@imanghafoori1

This comment has been minimized.

Copy link
Contributor

imanghafoori1 commented Jan 4, 2019

Where are the unit tests then?

@imanghafoori1

This comment has been minimized.

Copy link
Contributor

imanghafoori1 commented Jan 4, 2019

Does the logic exception make sense ?!
You may just return an empty array or something.
Exceptions are not good to throw to the users as I know.

@d3jn

This comment has been minimized.

Copy link
Contributor

d3jn commented Jan 4, 2019

@imanghafoori1

Does the logic exception make sense ?!
You may just return an empty array or something.
Exceptions are not good to throw to the users as I know.

LogicException is part of standard PHP library and to quote official documentation on the matter:

[It is] exception that represents error in the program logic. This kind of exception should lead directly to a fix in your code.

So it makes sense to be here because it indicates that you are trying to access parameters of yet to be bound route instance (which is a clear indication of an error in your code). It was already thrown when accessing unset parameters field, so it should also be thrown when trying to access original parameters too (because originalParameters just initialized with on-bound parameters value and is available only when parameters field is).

Where are the unit tests then?

Sure, will append this PR with tests. My bad.

@taylorotwell taylorotwell merged commit a2e0b92 into laravel:5.7 Jan 5, 2019

2 checks passed

continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@martinbean

This comment has been minimized.

Copy link
Contributor

martinbean commented Jan 13, 2019

@d3jn Is this for retrieving the value of a route parameter before it’s bound to a model, i.e. the original string representation?

@d3jn

This comment has been minimized.

Copy link
Contributor

d3jn commented Jan 13, 2019

@martinbean yes. It should come handy in case your resolution logic mutates the value in a way that it can no longer be retrieved from the resolved one or will require you to do additional queries/operations to do so (as it was in my case).

@martinbean

This comment has been minimized.

Copy link
Contributor

martinbean commented Jan 13, 2019

@d3jn Great. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment