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

Allow optional variadic arguments for methods/functions #337

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

yoramdelangen
Copy link
Contributor

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
Copy link
Collaborator

Xenira commented Nov 12, 2024

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

@Xenira
Copy link
Collaborator

Xenira commented Nov 13, 2024

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
Copy link
Contributor Author

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

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