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 PHP8 call_user_func_array breaking change #210

Closed
wants to merge 1 commit into from
Closed

Fix PHP8 call_user_func_array breaking change #210

wants to merge 1 commit into from

Conversation

PBillodeau
Copy link

Beginning with PHP 8, "args keys will now be interpreted as parameter names, instead of being silently ignored." https://www.php.net/manual/en/function.call-user-func-array.php

This solution effectively ignores named parameters in PHP8 by filtering out any named parameters. Not ideal, but it seems to work -- open to better ideas!

Beginning with PHP 8, "args keys will now be interpreted as parameter names, instead of being silently ignored." https://www.php.net/manual/en/function.call-user-func-array.php

This solution effectively ignores named parameters in PHP8 by filtering out any named parameters. Not ideal, but it seems to work -- open to better ideas!
@neekfenwick
Copy link

I think this is broken for named parameters that appear out of order.

As I recall, the original code might take a uri like /test/{foo}/{bar} and a request of /test/value/value2 and make a $params array like:

[
'0' => 'value',
'1' => 'value2',
'foo' => 'value',
'bar' => value2'
]

What you've done is reduce this array of params to:

[
'0' => 'value',
'1' => 'value2'
]

and then pass it to the handler function, but you've lost the ability to name the parameters, and if the handler function has them in a different order to how they appear in the URI, they will be passed to the wrong parameter.

For example:

/**
 * @uri /restTonic.php/test2/{foo}/{bar}
 */
class TestResource2 extends GenericResource {

    /**
     * @method GET
     * @provides application/json
     */
    public function get($bar, $foo) {
        global $l;

        $l->info("TestResource: foo $foo bar $bar");
    }

}

Note that the function takes $bar, $foo. Your code will pass the wrong values as seen in my logging output:

2022-11-04 04:21:12 INFO TestResource: foo value2 bar value

My fix at https://github.com/neekfenwick/tonic/tree/v3.4-alpha/src/Tonic does not have this problem.

As this project appears dead I have not bothered creating a PR. If we get any feedback from @peej or anyone with write permissions to this repo, I'll think about making one, but until then forking seems the way to go.

@PBillodeau
Copy link
Author

@neekfenwick, thank you for this thorough explanation! I agree -- your solution is superior.

@neekfenwick
Copy link

@neekfenwick, thank you for this thorough explanation! I agree -- your solution is superior.

I did have to run a test to check I was correct :) Are you thinking of using my repo? I'm not sure how 'official' to try to make it. For now I'm happy with a repositories entry in my composer.json that points my project to my github for peej/tonic instead of the officially published one. I don't particularly want to take over feature requests from the community that I see on this project, but we do at least need a functioning library for PHP 8 now that 7.x is going to lose security fixes this month.

@neekfenwick
Copy link

@PBillodeau this PR is still Open, are you going to close it?

@PBillodeau PBillodeau closed this Nov 13, 2022
@PBillodeau
Copy link
Author

@neekfenwick thanks for the reminder! PR closed. Right now I have our project setup to apply the patch you created using https://github.com/cweagans/composer-patches.

@neekfenwick
Copy link

@neekfenwick thanks for the reminder! PR closed. Right now I have our project setup to apply the patch you created using https://github.com/cweagans/composer-patches.

Ah, good stuff :) I'll probably make an official Release from my forked repo soon as we're probably rolling out my latest work tomorrow. Do you have any other patches you're applying that I should be aware of that might be useful adding to my repo?

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