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

Fix unable to use _remap without default method #1960

Closed
wants to merge 1 commit into from
Closed

Fix unable to use _remap without default method #1960

wants to merge 1 commit into from

Conversation

krishnan57474
Copy link

@krishnan57474 krishnan57474 commented Apr 21, 2019

Description
A controller with method other than _remap creates ReflectionException error in routes collector toolbar.
Fix for #1928

Checklist:

  • Securely signed commits

  • Component(s) with PHPdocs

  • Unit testing, with >80% coverage

  • User guide updated

  • Conforms to style guide

**Description**
A controller with method other than _remap creates ReflectionException error in routes collector toolbar.
Fix for #1928

**Checklist:**

    * [x]  Securely signed commits

    * [x]  Component(s) with PHPdocs

    * [ ]  Unit testing, with >80% coverage

    * [ ]  User guide updated

    * [x]  Conforms to style guide
@krishnan57474 krishnan57474 changed the title Fix unable to use _remap without default method #1928 Fix unable to use _remap without default method Apr 21, 2019
@@ -88,7 +88,7 @@ public function display(): array
$route = $router->getMatchedRoute();

// Get our parameters
$method = is_callable($router->controllerName()) ? new \ReflectionFunction($router->controllerName()) : new \ReflectionMethod($router->controllerName(), $router->methodName());
$method = is_callable($router->controllerName()) ? new \ReflectionFunction($router->controllerName()) : new \ReflectionMethod($router->controllerName(), method_exists($router->controllerName(), '_remap') ? '_remap' : $router->methodName());
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this will always go for remap...

Copy link
Author

Choose a reason for hiding this comment

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

nope it's not always go for remap. only when controller contains remap method. the main issue is, when controller contains remap method only, it creates error. $router->methodName() always return "index" when controller contains remap method only. if controller having both remap and index method, toolbar always choose index instead of remap. but router always prefers remap first. calling ReflectionMethod without checking method exists check creates ReflextionException error.

Copy link
Contributor

Choose a reason for hiding this comment

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

What I am saying here is,
for example, method abc() is present and I am calling same method from route, then also, according to your condition, if both method _remap() and abc() are present, it will always go for _remap().
But ideally, it should go directly to abc()

Copy link
Author

Choose a reason for hiding this comment

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

yup, you are right. but if you have controller with only one method, that is remap. then router correctly choose remap method, but toolbar always fallback to default method index, without checking method existence.

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