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

feat: add support for IN (array) condition with update() and delete() #6702

Open
wants to merge 1 commit into
base: 4.3.x
Choose a base branch
from

Conversation

simPod
Copy link
Contributor

@simPod simPod commented Jan 12, 2025

Q A
Type feature

Summary

This add support for WHERE IN (?, ?...) condition using $connection->update() and delete() methods.

I was looking for a reason it is not implemented yet in issues and tests but did not find any. IMO it's viable. 🤔

Copy link
Contributor Author

@simPod simPod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How to fix Line exceeds 120 characters violations

@greg0ire
Copy link
Member

Is using linebreaks not an option?

SInce this is a new feature, you should target 4.3.x

@simPod
Copy link
Contributor Author

simPod commented Jan 12, 2025

phpcs does not parse linebreaks in unions

@simPod simPod changed the base branch from 4.2.x to 4.3.x January 12, 2025 14:57
@greg0ire
Copy link
Member

What about linebreaks outside unions? I think it works for array values, for instance.

@simPod
Copy link
Contributor Author

simPod commented Jan 12, 2025

Yup, linebreaks within arrayshapes are supported. Linebreaking array<> syntax causes errors in other sniffs.

I used WrapperParameterType in @param for now. I don't see any potential issue in that.

* @param array<string, mixed> $criteria
* @param array<int<0,max>, string|ParameterType|Type>|array<string, string|ParameterType|Type> $types
* @param array<string, mixed> $criteria
* @param array<int<0,max>, WrapperParameterType>|array<string, WrapperParameterType> $types
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically, this is a breaking changes for extending classes, however I think it's not a big deal.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be documented though.

@greg0ire greg0ire requested review from morozov and derrabus January 12, 2025 16:03
$columns[] = $columnName;
$values[] = $value;
$conditions[] = $columnName . ' = ?';
if (is_array($value)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We cannot guess that an IN query must be used based on the value being an array. This would break types that map to a PHP array like JSON for instance.

@derrabus
Copy link
Member

We should add a functional test instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants