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

Passing a single argument to the QueryBuilder::where() method using named arguments #11253

Closed
deasraze opened this issue Feb 13, 2024 · 7 comments

Comments

@deasraze
Copy link

Bug Report

Q A
BC Break yes
Version 3.0.0

Summary

Hello there! When we try to call the QueryBuilder::where() method using a named argument, we encounter an issue with accessing an undefined array key. As a result, we get a "Warning: Undefined array key 0". The problem occurs because the value of the argument is an associative array.

if (! (count($predicates) === 1 && $predicates[0] instanceof Expr\Composite)) {

Current behavior

Warning: Undefined array key 0

How to reproduce

Сall the QueryBuilder::where() method using a named argument:

$qb->where(predicates: 't.column = :value')

Expected behavior

The method worked without errors.

@derrabus
Copy link
Member

derrabus commented Feb 13, 2024

You cannot target a variadic parameter with named arguments. That's a limitation of PHP and we can't do much about it.

The best we could do here is to throw if we detect unexpected named arguments in $predicates. Not sure if it's worth it though.

@derrabus derrabus closed this as not planned Won't fix, can't repro, duplicate, stale Feb 13, 2024
@deasraze
Copy link
Author

deasraze commented Feb 13, 2024

That's a limitation of PHP and we can't do much about it.

Sorry, but PHP doesn't prohibit the use of named arguments in conjunction with variadic parameter, as long as only one argument is passed at a time. Due to the fact that only one argument is being used, this piece of code is functioning properly.
3v4l example

<?php

declare(strict_types=1);

function foo(mixed ...$variadic): void
{
    var_dump($variadic);
}

foo(variadic: 'some');

Calling the QueryBuilder::select() method work without errors when using $qb->select(select: 't.column').
It seems to me that a possible solution in this situation would be to at least ban the use of named arguments with the @no-named-arguments or fix a bug that does not take this scenario into account.

@derrabus
Copy link
Member

Sorry, but PHP doesn't prohibit the use of named arguments in conjunction with variadic parameter,

I didn't say that it prohibits it. I said, it doesn't support it.

this piece of code is functioning properly.

It's not and you can see that in the output. You're not targeting the $variadic parameter with the named argument variadic:. Instead, PHP passes all arguments (ordered or named) that it could not match to any parameter (which includes your variadic: argument) as an array to the variadic parameter. This becomes clearer if you add more arguments to the call:

https://3v4l.org/LSYaK

So, your example code does something, but clearly not what you think it would.

It seems to me that a possible solution in this situation would be to at least ban the use of named arguments with the @no-named-arguments

Be my guest. 🙂

@deasraze
Copy link
Author

It seems that we misunderstood each other due to my poor English :(
From the PHP perspective, we can write foo(variadic: 'some') but we cannot write foo(variadic: 'some', variadic: 'other'). In the first case and based on your example, the code would be correct, as PHP would be able to execute it, but if we were to call where(predicates: 't.column= :value') or where(column: 't.column = :value'), we would fail because the code tries to access a non-existent key. It's not obvious.

Thanks.

@derrabus
Copy link
Member

If a method has a variadic signature, you can (in addition to the mandatory parameters) pass any number of ordered or named arguments you like to that method. The question is, does it make sense semantically and what do you expect to happen when you do that? Just because you can, does it mean you should?

Your expectation was:

The method worked without errors.

I'm disputing that because we never documented that we would expect arbitrary named arguments on this method. I understand that the error message is more of the garbage-in-garbage-out type and we can certainly improve that. But if we do, it will still be an error, that's for sure.

@derrabus
Copy link
Member

see #11260

@deasraze
Copy link
Author

This will really help improve the DX and help prevent possible problems if we are expecting to receive the value of an argument in the form of a list<mixed> type. Thank you.

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

No branches or pull requests

2 participants