fix: calling order of optional parameter

[1e1]

! tested only on PHP 5.3 !

While a route catch parameters in a certain order AND the relative method declares its parameters in another order AND all optional paramters are not setted, the parameters are not mapped in the correct order.

Route: #^/startups(?:/(?<provider>[\w\-]+)(?:/(?<domain>[\w\-]+))?)?/(?:§(?<page>\d+))?$# Method: ($page = 0, $provider = null, $domain = null) Url: /startups/aProvider/aDomain -> call: f('aProvider', 'aDomain', --null--)

My user case occured in a controller which is extended by 2 psecific controllers.

[1e1]

In this case, we should use an ordered iterator: reset/next instead of foreach

[Hywan]

I assign @osaris on this PR :-).

[Hywan]

ping :-) ?

[osaris]

Hello,

Sorry for the delay, I'm trying to test your PR but I can't recreate the bug with your informations (I can't use your regex to create Hoa/Router rule as is). Can you please give me your full route declaration ?

[osaris]

Ok, I finally manage to reproduce the problem. If I reorder the function parameters to match the parameters in the regex it's ok. If I use your patch, I have the following error :

Fatal error: Uncaught exception 'ReflectionException' with message 'Cannot determine default value for internal functions' in /Users/raphaelemourgeon/Sites/hoa-dispatcher-test/vendor/hoa/dispatcher/Hoa/Dispatcher/Basic.php:126 Stack trace: #0 /Users/raphaelemourgeon/Sites/hoa-dispatcher-test/vendor/hoa/dispatcher/Hoa/Dispatcher/Basic.php(126): ReflectionParameter->getDefaultValue() #1 /Users/raphaelemourgeon/Sites/hoa-dispatcher-test/vendor/hoa/dispatcher/Hoa/Dispatcher/Dispatcher.php(157): Hoa\Dispatcher\Basic->resolve(Array, Object(Hoa\Router\Http), NULL) #2 /Users/raphaelemourgeon/Sites/hoa-dispatcher-test/index.php(20): Hoa\Dispatcher\Dispatcher->dispatch(Object(Hoa\Router\Http)) #3 {main} thrown in /Users/raphaelemourgeon/Sites/hoa-dispatcher-test/vendor/hoa/dispatcher/Hoa/Dispatcher/Basic.php on line 126

when I try to call : /startups/aProvider/domain/ and with the following router code :

$router->get('z', '/startups(?:/(?<provider>[\w\-]+)(?:/(?<domain>[\w\-]+))?)?/(?:§(?<page>\d+))?',
                          function($page = 0, $provider = null, $domain = null) {

  echo $page . '/' . $provider . '/' . $domain;
});

But I can't understand why parameter order has this side effect as we are only relying on parameter name to set values and default value is handled by standard PHP default value for function parameters ?

[Hywan]

Still opened?

[Hywan]

@osaris: ping?

[osaris]

@1e1 ping ?

[1e1]

Hi,

I have not re-read the code. I remember that if I defined a method with optional parameters C::m($year=null, $month=null, $day=null), then if I defined 2 routes:

• $router->get('date_fr', '/fr/(?<day>\d{2})?-(?<month>\d{2})?-(?<year>\d{4})?', 'C', 'm');
• $router->get('date_en', '/en/(?<month>\d{2})?-(?<day>\d{2})?-(?<year>\d{4})?', 'C', 'm');
• $router->get('date_iso', '/(?<year>\d{4})?-(?<month>\d{2})?-(?<day>\d{2})?', 'C', 'm');

Only /2015-07- will be correct. /fr/-07-2015 and /fr/07--2015 will call C::m(07, 2015, null)

"why parameter order has this side effect" Even if the parameter is optional we have to set it with the default value, otherwise it is not present and the parameter list is sliced.

[osaris]

I have reproduced the problem, if the last part of the regex is empty then the $arguments array doesn't include values for missing parameters. For example, if you test /fr/--2015 it works well because $arguments contains a value for each variables (empty for missing values). But if you test /fr/01-- the $arguments array contains only a value for day.

As invokeArgs use params by order not by name, then it affiliates the first and only params (day) to the first parameter of the method.

Your patch is ok for me, it adds missing parameters to the $argumentsarray so all parameters (even optionals) are passed to the called method.

[Hywan]

Is it possible to rebase the patch to master please? (git rebase master might do the job)

[1e1]

Sorry for the delay. I have just updated the PR. Then we should run PHP-CS-fixer, there is a lot of Checkstyle mistakes.

[Hywan]

@1e1 Thanks!

Can I suggest you:

$ composer install
$ vendor/bin/hoa devtools:cs --diff .

As mentionned here: http://hoa-project.net/En/Literature/Contributor/Guide.html#Preparing_a_patch.

Waiting for your go.

[1e1]

No more changes. Go!

[Hywan]

@1e1 Please, can you rebase your commits now :-)? One commit will be fine.

[Hywan]

The error should probably exists on Hoa\Dispatcher\ClassMethod. To merge this PR, I would like to know if this is possible to apply the same patch on Hoa\Dispatcher\ClassMethod but also to write a test for that particular case.

Is it possible?

[osaris]

@Hywan you asked for a review of this PR on IRC last week, It seems that :

• we lack a test to cover the problem addressed by this feature
• we lack a test to ensure the problem doesn't exists on ClassMethod dispatcher
• we may lack a fix in the ClassMethod dispatcher if the test reveal that the bug also exists here

@1e1 can you please add a test for your PR and the same test for the ClassMethod disptacher ? We are here to help if you need !

[Hywan]

ping?

[Hywan]

I don't know how to apologize for the delay…