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

SimpleRouter: fix compatiblity with non-stringable (e.g. Closure) default parameter values #9

Merged
merged 1 commit into from
Feb 6, 2021

Conversation

xificurk
Copy link
Contributor

@xificurk xificurk commented Dec 6, 2020

  • bug fix? yes
  • new feature? no
  • BC break? no

The bug was intoroduced in d989c52. SimpleRouter should support non-stringable (e.g. Closure instance) default parameter values - a typical use case is Nette:Micro presenter.

@dg dg force-pushed the master branch 4 times, most recently from cfc5e09 to 9f95c3b Compare February 6, 2021 00:53
@dg dg merged commit 1116db2 into nette:master Feb 6, 2021
dg pushed a commit that referenced this pull request Feb 6, 2021
@dg
Copy link
Member

dg commented Feb 6, 2021

I changed it that this will work since PHP 8, because == behaves too loosely in < 8.

dg pushed a commit that referenced this pull request Feb 6, 2021
@xificurk
Copy link
Contributor Author

xificurk commented Feb 6, 2021

@dg Does this mean you do not intend to support routing of requests to MicroPresenter using SimpleRouter on PHP < 8?

I am not sure we understood each other. This use-case was originally supported, but it was broken by changes in d989c52. The problem is that MicroPresenter requires closure callback parameter, so the parameter value is non-stringable object and URL construction crashes on this if you try to use SimpleRouter.

@dg
Copy link
Member

dg commented Feb 6, 2021

I get it now.

@dg
Copy link
Member

dg commented Feb 6, 2021

No. I still don't get it :-)

Can you give me a realworld example where constructUrl() is called and the parameter is closure?

@xificurk
Copy link
Contributor Author

xificurk commented Feb 6, 2021

The original context for this PR is in this thread: https://pehapkari.slack.com/archives/C2R30BLKA/p1607199796381500

Here is example on top of current sandbox:
https://github.com/xificurk/sandbox/tree/simplerouter-bug-example
error

@dg
Copy link
Member

dg commented Feb 8, 2021

Thanks

dg added a commit to nette/application that referenced this pull request Feb 8, 2021
dg added a commit to nette/application that referenced this pull request Feb 8, 2021
dg added a commit that referenced this pull request Feb 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants