Skip to content

Fetch the last insert ID only with INSERT statements. #2347

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.

Sign up for GitHub

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

Jump to bottom
Closed
tobiastom wants to merge 1 commit into from
Closed

Fetch the last insert ID only with INSERT statements. #2347

tobiastom wants to merge 1 commit into from

Conversation

@tobiastom
Copy link
Contributor

@tobiastom tobiastom commented Dec 11, 2019

Describe the PR

I did implement a Postgresql Adapter for Kirby. PostgreSQL does not seem to like when the last insert ID is requested, but it was for example a SELECT query.

This PULL request changes the call to lastInsertId on INSERT queries.

I'm aware that this is not a nice solution, but based on the context that is available at that given point, it seems to be the only one.

Ready?

  • Added unit tests for fixed bug/feature
  • Passing all unit tests
  • Fixed code style issues with CS fixer and composer fix
  • Added in-code documentation (if needed)

@lukasbestle
Copy link
Member

lukasbestle commented Dec 12, 2019

Thanks for the PR. I actually think that the solution is fine if it’s necessary. However we should probably use case-insensitive matching with Str::startsWith($query, 'insert ', true).

Out of interest: Which kind of error did you get?

We’d also be happy to include the Postgres adapter in the core if you don’t mind sharing it.

@lukasbestle lukasbestle self-assigned this Dec 12, 2019
@lukasbestle lukasbestle added this to the 3.4.0 milestone Dec 12, 2019
@tobiastom
Copy link
Contributor Author

tobiastom commented Dec 16, 2019

@lukasbestle I'll get back to you as soon as possible. I'm quite busy right now, sorry. :)

@lukasbestle
Copy link
Member

lukasbestle commented Feb 23, 2020

As we are refactoring the Database classes right now, I have added this fix to the current refactoring state as well. The refactored version will hopefully be in v3.4.0.

Thanks again for your PR! 👍

@tobiastom
Copy link
Contributor Author

tobiastom commented Feb 24, 2020

Thanks for integrating it and sorry that I did not find the time to report back the error. We switched to the native PDO classes inside the project where I needed this fix.

@tobiastom tobiastom deleted the bugfix/last-insert-id branch February 24, 2020 16:42
@distantnative distantnative removed this from the 3.4.0 milestone Mar 6, 2020
@lukasbestle lukasbestle added this to the 3.4.0 milestone Apr 25, 2020
lukasbestle added a commit that referenced this pull request Apr 28, 2020
@lukasbestle @tobiastom
Database: Only fetch insert ID for inserts
69eb061 
#2347

Co-Authored-By: Tobias Tom <[email protected]>
@lukasbestle lukasbestle mentioned this pull request Apr 28, 2020
Merged
4 tasks
bastianallgeier pushed a commit that referenced this pull request May 4, 2020
@lukasbestle @tobiastom @bastianallgeier
Database: Only fetch insert ID for inserts
4c72c9f 
#2347

Co-Authored-By: Tobias Tom <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@lukasbestle lukasbestle

Labels

None yet

Milestone

3.4.0

Development

Successfully merging this pull request may close these issues.

3 participants

@tobiastom @lukasbestle @distantnative