Allow optional variadic arguments for methods/functions

[yoramdelangen]

I was implementing the variadic arguments into our extension allow for the following signatures:

// no arguments - impl 1
$client->query('SELECT * FROM Employees');

// one or more arguments
$client->query('SELECT * FROM Employees WHERE Employee_ID = ?', 1);
$client->query('SELECT * FROM Employees WHERE Employee_ID = ? OR Employee_ID = ?', 1, 2);
$client->query('SELECT * FROM Employees WHERE Employee_ID = ? OR Employee_ID = ?', [1, 2]);

// reference arguments - impl 2
$a = 1;
$client->query('SELECT * FROM Employees WHERE Employee_ID = ?', $a);
$b = [1, 2];
$client->query('SELECT * FROM Employees WHERE Employee_ID = ? OR Employee_ID = ?', $b);
$client->query('SELECT * FROM Employees WHERE Employee_ID = ? OR Employee_ID = ?', ...$b);
$client->query('SELECT * FROM Employees WHERE Employee_ID = ? OR Employee_ID = ?', ...[1, 2]);

This raised multiple issues for me:

1. "impl 1": macro #[optional(params)] parameter forces you to set defaults. But only literals are allowed, so vec![], Vec::new(), None etc. do not work.
2. "impl 1": params: Option<&[&Zval]> it worked.. kinda.. but raised 2 problems:
   • only allowed referenced parameters/arguments via variables, dunno why.
   • removed Option<...>, and then this missed the implementation for FromZvalMut for &[&Zval]. So I added implementation, in the PR because its nice allow handle for Option<&[&Zval]>.

Fixes

1. Resolved by checking when requiring defaults for optional arguments if its not variadic.
2. Resolved by adding the FromZvalMut implementation for &[&Zval].

[Xenira]

Thanks for the contribution. Will have a look this evening.

[Xenira]

Sorry, for the delay.
Had a look at this and another alternative to consider would be to allow other tokens and not only literals in the default macro.
Haven't found the time to actually recreate this locally as my week was busier than expected, but expect to have a closer look tomorrow or friday.

Also you seem to need to run cargo fmt.

[yoramdelangen]

@Xenira thanks for reaching out! I have pushed code formatting into the branch, all checks should pass now!

[Xenira]

Hey @yoramdelangen sorry again for the delay.

Maybe I did somethitg wrong, but i do not get the expected result:

$client->query('SELECT * FROM Employees WHERE Employee_ID = ?', $a);
// "SELECT * FROM Employees WHERE Employee_ID = ?" None

$client->query('SELECT * FROM Employees WHERE Employee_ID = ? OR Employee_ID = ?', ...$b);
// PHP Fatal error:  Uncaught ArgumentCountError: hello_world() expects at most 2 arguments, 3

Could you add some tests to clarify this?

[yoramdelangen]

@Xenira I will extend/create a test for it

[yoramdelangen]

@Xenira what is the best place to put these tests? Cannot really find a fitting spot for it

[Xenira]

@yoramdelangen easiest would be here https://github.com/davidcole1340/ext-php-rs/tree/master/tests/src/integration

[yoramdelangen]

I have added tests and extended the docs reflecting the option of optional args, by default it requires you to have arguments added. All remarks are welcome!

Now fixing the pipelines!

[Xenira]

Now fixing the pipelines!

Might need to rebase on master, as some clippy rules changed.

[yoramdelangen]

Fixed!

[Xenira]

Thanks for adding the tests. Really appreciate it.

Didn't have the time to look at everything in detail yet, but this is what I noticed at a first glance. Will check it out in the next few days.

[yoramdelangen]

Updated PR with latest master branch changes..

[yoramdelangen]

Whoop, i just noticed my latest changes were done based upon PHP 8.4, I added the missing fields and missing cfg php84 to make sure the changes only reflect in these version.

Is this the way to resolve it, or should I undo everything? Tests were running fine with no problems

[Xenira]

Hey @yoramdelangen based on a quick glance 739967e should have been a rebase on master.

Regardless there seems to still be a issue with the integration tests. You can run just those by running cargo test inside the tests directory.

[yoramdelangen]

Undid the merge and rebased master ontop. Ran clippy and resolved it as well.

Tests were failing due to unused parameter in function _numbers where the optional flag stated "numbers" that failed.

[Xenira]

@yoramdelangen Hey, there seem to be conflicts again.

Tried to rebase locally to check out whats wrong, and it seems its because of the php8.4 stuff.
From what I can see it looks fine now, but would like to see a clean history to be sure.

If you rebase, you likely need to adjust the commit messages of your commits to conventional commits, as the pipeline checks for those now.

[Xenira]

@yoramdelangen i fixed this in f819927

Part of a major rework of the macro system.

I will close this now, but thank you so much for your work. Helped me a lot getting this done.

[yoramdelangen]

Thank you for keeping the project working!

[Xenira]

@yoramdelangen could you try the macro-state-deprecation-v2 branch and let me know if everything works for you?

[yoramdelangen]

@yoramdelangen could you try the macro-state-deprecation-v2 branch and let me know if everything works for you?

Yes, it still works!

[Xenira]

@yoramdelangen thank you. Will try to get that into master soon. But will be a bit before its released as this is a major change and I expect some bugs.