PHP >= 7.3

[DeveloperMarius]

Hey,

require PHP 7.3 and upgrade to PHPUnit tests v8.5 in the next major release. PHP 7.1 isn't supported anymore.

[skipperbent]

Do you think the next major release should support php8 or is that still too soon?

[DeveloperMarius]

Hey,

I'm using PHP 8 in all my projects and I love the new union types, the str_contains, starts_with, ends_with functions and the new attributes for No return or ArrayShape that is integrated by JetBrains. But there are a view problems:

• the most projects doesn't support PHP 8 now
• The imagick libary is going to need years to get updated
• I have to add to every composer update the --ignore-platform-reqs option
• etc.

But that concerns reference to the upgrade to unsupport PHP 7. But you have in mind to add PHP 8 support and this would be a great idea. Should I add it like this to composer?:

"require": {
    "php": "^7.3||^8"
}

[DeveloperMarius]

I will test to add it to github actions but I had some errors white testing the first time. So don't worry about the upcomming errors in the checks.

[DeveloperMarius]

There are a lot of errors with testing in php 8. I will look into it.

[DeveloperMarius]

Okay, it seems to be somthing deeper. Here is one error (they all look very similar and I think it's one problem) maby you have an idea, but I will search threw the code now:

 1) ClassLoaderTest::testCustomClassLoader
Error: Unknown named parameter $result

/home/runner/work/simple-php-router/simple-php-router/tests/Pecee/SimpleRouter/Dummy/ClassLoader/CustomClassLoader.php:12
/home/runner/work/simple-php-router/simple-php-router/src/Pecee/SimpleRouter/Route/Route.php:81
/home/runner/work/simple-php-router/simple-php-router/src/Pecee/SimpleRouter/Router.php:409
/home/runner/work/simple-php-router/simple-php-router/src/Pecee/SimpleRouter/Router.php:338
/home/runner/work/simple-php-router/simple-php-router/src/Pecee/SimpleRouter/SimpleRouter.php:66
/home/runner/work/simple-php-router/simple-php-router/tests/TestRouter.php:18
/home/runner/work/simple-php-router/simple-php-router/tests/TestRouter.php:24
/home/runner/work/simple-php-router/simple-php-router/tests/TestRouter.php:42
/home/runner/work/simple-php-router/simple-php-router/tests/Pecee/SimpleRouter/ClassLoaderTest.php:22

[DeveloperMarius]

I'm really don't get what is going on there.

ClassLoaderTest::testCustomClassLoader
Error: Unknown named parameter $result

the function:

public function testCustomClassLoader()
{
    $result = false;

    TestRouter::setCustomClassLoader(new CustomClassLoader());

    TestRouter::get('/', 'NonExistingClass@method3');
    TestRouter::get('/test-closure', function($status) use(&$result) {
        $result = $status;
    });

    $classLoaderClass = TestRouter::debugOutput('/', 'get', false);
    TestRouter::debugOutput('/test-closure');

    $this->assertEquals('method3', $classLoaderClass);
    $this->assertTrue($result);

    TestRouter::router()->reset();
}

I don't get where the $status should come from. The route '/test-closure' has no parameter named status? @skipperbent where does $status come from?

~ Marius

[skipperbent]

Hi.

Status is passed on as argument for the callback function($status).

The argument is generated by the custom-class loader - if status is true it means that the class-loader successfully manipulated the output and it will set the global $result with the returned value.

TestRouter::setCustomClassLoader(new CustomClassLoader());

See tests/Pecee/SimpleRouter/Dummy/ClassLoader/DummyClassLoaderTest.php

class CustomClassLoader implements \Pecee\SimpleRouter\ClassLoader\IClassLoader
{
    public function loadClass(string $class)
    {
        return new DummyController();
    }

    public function loadClosure(callable $closure, array $parameters)
    {
        return \call_user_func_array($closure, ['result' => true]);
    }
}

[DeveloperMarius]

Oh, yes. Now I see it. Thank you.

[skipperbent]

Np 😄 The classloader can manipulate how the router should handle classes + methods. They control how classes and methods are loaded. Perfect for people that want to use custom injection or autoload.

The example in the unit-test is only for test which makes it a bit confusing.

Here how it looks when using a custom classloader with php-di for dependency injection: https://github.com/skipperbent/simple-php-router/blob/22d531178a7874a9a817ecd307e73fece498fd96/README.md#custom-class-loader

[DeveloperMarius]

Okay, I can't get it working for now. I'm confused because the router wokes with php 8 on my server. I will focus on other changes for now, becuase this will just be important when we prepare the major update.

[DeveloperMarius]

Hey @skipperbent,

First of all I fixed the error in ClassLoaderTest@testCustomClassLoader with renaming the param in CustomClassLoader@loadClosure from result to status. Because result is the parameter "used use(&$result)" in the function and $status is the real parameter. This throws an error now, because the array keys are now used as parameter names in PHP 8 and the parameter result is not used in the function.

Now to the other errors which are basically the same, but I wanted to ask you if this is intentionally: For this example I use the RouterPartialGroupTest@testParameters test. When a partialGroup is called you can use parameters. This is normal so far. But when you add routes inside of them you have to add these parameters also to all of their functions. So for example:

TestRouter::partialGroup('{param1}/{param2}', function ($param1 = null, $param2 = null){
   TestRouter::get('/', function($param1 = null, $param2 = null){
      echo 'test';
   });
}

Currenty you don't do that with your DummyController:

TestRouter::partialGroup('{param1}/{param2}', function ($param1 = null, $param2 = null){
   TestRouter::get('/', 'DummyController@method1');
}

and public function method1(){}

To fix this issue I have to create a new public function method4($param1, $param2) in the DummyController. Do you understand what I mean? Should I do this?

~ Marius

[skipperbent]

php8 is backwards compatible, so it should work. But things can be optimized to take advantage of the php8 features, but I'm not sure if switching to php8 would do more harm than good since it might make it incompatible with peoples projects as it takes time for new versions of php to go mainstream.

It's intentional - it's not errors. There has to be a unit-test for every possible scenario, that's how we can ensure that every feature works as expected. The test you are confused about is testing the route can load with empty parameters.

[DeveloperMarius]

Do you mean the first or the second described error?

[skipperbent]

The second. The one with questions about the php-unit test using partialGroup :)

[DeveloperMarius]

But the route cannot load with empty parameters. Because php call_user_func_array puts the parameter into the function using "named parameters" as described here. When there are not parameters, the named parameters match nothing and they throw an Error: Unknown named parameter. My question is, if we should implement a function that only privides parameters that are requested in the function or require that all parameters are in the function?

So option 1:

TestRouter::partialGroup('{param1}/{param2}', function ($param1 = null, $param2 = null){
   TestRouter::get('/', function($param1 = null){
      echo 'test';
   });
}

we check what of the parameters ["param1", "param2"] are requested. Only $param1 is requested so we only call call_user_func_array with $param1.

Option 2:

TestRouter::partialGroup('{param1}/{param2}', function ($param1 = null, $param2 = null){
   TestRouter::get('/', function($param1 = null, $param2 = null){
      echo 'test';
   });
}

like I described in my last comment, we require $param1 and $param2 to be in the function. If one or more of them are not, PHP will throw an error.

php8 is backwards compatible

Yes it is, because in PHP 7 this should have thrown an error too, because we provide parameters to a function which didn't requested it. In this case PHP 7 didn't throw an error.

Which option should we use?

You can see the errors in the Checks tab under PHPUnit Tests (ubuntu-latest, 8.1, 9.5.4, lowest) -> Run tests with phpunit

An other question: Is my fix for testCustomClassLoader okay?

~ Marius

[DeveloperMarius]

I fixed the issues discribed above. Ther is just one left with the Resource tests. I don't know where the $id parameter is. I will look into it the next days.

[DeveloperMarius]

Okay, I fixed the buggs using ReflectionFunction and ReflectionMethod. I know you don't like them and I understand that, but it's the only way to validate the parameters. I added the option $processParametersSafe which will check on function call if the given parameters match the controller function / callable. When there are $parameters which aren't in the function, it will remove them and if the function requires parameters, which aren't in the $parameters, it adds them with an value of null.

I will now try to improve the performance of this check and I already have some ideas. Updates will follow.

~ Marius

[skipperbent]

Hmm it's a pretty big behavioral change it's odd that it's not even mentioned on php.net except for the comments :)

I think parameters are always sent in the correct position from the router and from what I can read from the comments call_user_func_array supports positional parameters.

Try to see if this will work:

return \call_user_func_array($closure, array_values($parameters));

/ Simon

[DeveloperMarius]

Sure I will try it

[skipperbent]

You can just checkout v4-development and change it there so you don't have to revert all your code. Not sure if it will work, but worth a try.

[DeveloperMarius]

omg, I'm sooo dumb. Didn't thought of that and worked for so long on fixing this... Thank you, great fix! What do you mean with "checkout v4-development"? Should I change v4-development directly or what?

[skipperbent]

The PHP 8 change of call_user_func_array is a bit lacking. How are you supposed to catch the error, there's no function to test if the parameters exists before calling it (except the reflection class) - so you can't even throw your own exception.

I'm glad you got it working. It was only by chance I saw one guy in the comments on php.net demonstrating the behavior on PHP8.

It was in case it didn't work i thought maybe it was easier just to change repo like git checkout v4-development and do the change instead of reverting all your code in case it didn't work :)

[DeveloperMarius]

Ah, I reverted the changes manually^^ Everything should work now 👍🏻 You can catch the error like I did by catching \Error and then check the error message for the pattern if (preg_match("/^Unknown named parameter/", $e->getMessage())) {. But this is also not so beautiful...

~ Marius

[skipperbent]

Ok so it does throw an exception? At least that's something. It doesn't mention any of this in php.net. It just says "returns false on error". But still, they should make it possible for developers to avoid the error by adding a function that checks if parameters exist. Maybe it's a bug who knows, at least we found a workaround haha

[DeveloperMarius]

But I have to say that I don't really like this solution. Let's take a look at the following example:

TestRouter::partialGroup(
            '/lang/{test}/',
            function ($lang = 'en') use($route1, $route2, $route3) {
          TestRouter::get('/test/{data}', function ($data) use($route1) {
                    return $data;
          });
});

if the request is: /lang/de/test/hallo the user would return "de". And this is in my opinion not right. I would like an option like $namedParameters that enables my validation on a basic level and checks if the length of the given $parameters matches the length of the required parameters. If they don't I would set all missing $parameters to null and would remove the given $parameters that are not required. With that we have a basic little validation and when the size is equal the validation is passed. We can set the default to false and use your way, but on some point this will cause errors.

[skipperbent]

Data will return de? Can't find $user. In that case the parameter parsing is not setting the value correctly.

It's a bad solution because php changed the behavior of something without the tools to revert to the old behavior and now we're stuck with a hammer! Seems like a bug.

Nevertheless let's change the parameter parsing so the values are always available in the correct order. Does the test above fail?

[DeveloperMarius]

Can't find $user

What do you mean? The example above will print "de" and not the data value "hallo". This is not one of your tests, just an example where this will lead to buggs/ errors.

Okay, so you want to stay with call_user_func_array($closure, array_values($parameters)); and just force the users to use the right order for the values? But when we do that, there is no sence in naming the parameters in the url correct: '/test/{data}' and '/test/{ddddd}' can be accessed using function($data) as callback. Or we revert it to call_user_func_array($closure, $parameters); and force the user to use all $parameters in the functions.

(I still think my concept is a better solution. It's not beautiful but what are Reflection functions for if not for this.)

~ Marius

[skipperbent]

You wrote

if the request is: /lang/de/test/hallo the user would return "de".

I guess you mean data.

No, I'm saying there's an error in the way parameters are parsed if they are not available when using array_values.

I've just tested the test you provided on version 4.3 and I get the same error. I'll release a fix shortly.

[DeveloperMarius]

Oh, sry. The function will return, yes.

How are you planning to fix this without Reflection? We want to use data from the group in our request callback, but you have to detect what the developer wants to use in his callback function. I did this in here using:

$reflection = new \ReflectionMethod($classOrCallable, $method);
$functionParameters = array();
$missingParameters = array();
$removeParameters = array();
foreach($reflection->getParameters() as $parameter){
    $functionParameters[$parameter->getName()] = $parameter->isOptional();
    if(!$parameter->isOptional() && !in_array($parameter->getName(), array_keys($parameters))){
        $missingParameters[] = $parameter->getName();
    }
}
foreach(array_keys($parameters) as $parameter){
    if(!in_array($parameter, array_keys($functionParameters))){
        $removeParameters[] = $parameter;
    }
}

Now we have the missing $parameters that the user requested in the callback function, but who are not provided by the router and the $parameters who have to be removed from the $parameters that are parsed into the callback function.

[skipperbent]

It wasn't working in version 4.3 either without the array_values workaround. It was caused by a problem with parameters inherited by groups. I've fixed the issue here and changed the ClassLoader to use array_values.

https://github.com/skipperbent/simple-php-router/pull/526/commits/8eba5ab3d55d09700a86f94ea376ffda808097cb

Using ReflectionClass for this is more like a hotfix than a real-fix. Reflection-class is resource heavy.