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

Extend QueryBuilder for possibility to use "into" and "from" with sta… #6114

Closed
wants to merge 1 commit into from

Conversation

d3mo17
Copy link

@d3mo17 d3mo17 commented Jul 26, 2023

…tements "insert" and "delete"

Q A
Type improvement

Summary

It can be confusing, for example when $qb->delete()->from('users')->where("id = 5")->executeStatement(); has no effect in the database and the user entry will not be removed.
This change should bring more consistency to the Querybuilder.

@derrabus derrabus changed the base branch from 3.6.x to 3.7.x September 18, 2023 11:11
@derrabus
Copy link
Member

Thank you for the PR. I understand that delete()->from(…) looks correct and might be a common pitfall. But I'm not sold on adding an into() method.

@@ -585,7 +586,7 @@ public function getMaxResults()
/**
* Either appends to or replaces a single, generic query part.
*
* The available parts are: 'select', 'from', 'set', 'where',
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason for this change? What happens to existing apps that call $qb->add('set', …)?

@@ -777,18 +776,13 @@ public function delete($delete = null, $alias = null)
*
* @return $this This QueryBuilder instance.
*/
public function update($update = null, $alias = null)
Copy link
Member

Choose a reason for hiding this comment

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

Apps that currently calls $qb->update() without parameters will break after your change.

Comment on lines -841 to -844
return $this->add('from', [
'table' => $from,
'alias' => $alias,
], true);
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason for not calling add() anymore?

Copy link

There hasn't been any activity on this pull request in the past 90 days, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 7 days.
If you want to continue working on it, please leave a comment.

@github-actions github-actions bot added the Stale label Dec 18, 2023
@derrabus
Copy link
Member

Let's close. We've never heard back from the contributor.

@derrabus derrabus closed this Dec 18, 2023
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.

2 participants