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

fix: invalid INSERT/DELETE query when Query Builder uses table alias #5376

Merged
merged 4 commits into from
Jul 14, 2022

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Nov 23, 2021

Description
Fixes #5365

  • remove alias from FROM string when INSERT/DELETE

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@kenjis kenjis added bug Verified issues on the current code behavior or pull requests that will fix them database Issues or pull requests that affect the database layer labels Nov 23, 2021
@iRedds
Copy link
Collaborator

iRedds commented Nov 23, 2021

I have nothing against the motive of this PR.
But it starts to smell like crutches.

@lonnieezell
Copy link
Member

I agree. This feels like we're putting a bandaid on an error instead of fixing a root cause.

@MGatner
Copy link
Member

MGatner commented Jul 9, 2022

Checking back in on this... I'm all for deeper fixes but we have an open bug with a working solution which seems like a good merge to me. I assume the "do it right" approach would involve a fair amount of class refactoring around conditional aliasing, or something like that - and the trend has been to stay away from large changes to the database layer.

@kenjis
Copy link
Member Author

kenjis commented Jul 11, 2022

I think if a PR makes CI4 better, we should merge it.
Of course, if the quality is too low or the technical debt is too great, it should be rejected.

If you have a better solution, you should send another PR.
I think it is not better to leave a bug unfixed for months.

Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

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

I agree. This doesn't lock us out from a refactor fix so even if it is a bandaid it will stop some of the bleeding 😅 Good by me.

@kenjis
Copy link
Member Author

kenjis commented Jul 12, 2022

Added @internal and "This is a temporary solution." in removeAlias() method doc comment.

@kenjis kenjis merged commit 72963d0 into codeigniter4:develop Jul 14, 2022
@kenjis kenjis deleted the fix-5365-builder-insert branch July 14, 2022 00:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Verified issues on the current code behavior or pull requests that will fix them database Issues or pull requests that affect the database layer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: [QueryBuilder] Invalid INSERT query when builder use table alias
5 participants