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

Dev: Make getInsertID() more consistent with save() #9233

Open
evansharp opened this issue Oct 22, 2024 · 1 comment
Open

Dev: Make getInsertID() more consistent with save() #9233

evansharp opened this issue Oct 22, 2024 · 1 comment

Comments

@evansharp
Copy link
Contributor

PHP Version

8.2, 8.3

CodeIgniter4 Version

latest

CodeIgniter4 Installation Method

Composer (using codeigniter4/appstarter)

Which operating systems have you tested for this bug?

macOS, Linux

Which server did you use?

fpm-fcgi

Database

MaraiaDB

What happened?

Not exactly a bug, but not really a FR either. See this forum thread: https://forum.codeigniter.com/showthread.php?tid=91800&pid=421782#pid421782

Essentially I'm offering to PR a change to \System\Model::update() that would set an instance's $insertID property the same way \System\Model::insert() does so that the behaviour of save() is more consistent.

Currently \System\Model::getInsertID() only returns a value if a save() does an insert. The logic needed to handle that works against the elegance provided by save().

Steps to Reproduce

Use \System\Model::save()

Expected Output

Best case, \System\Model::getInsertID() will return the "affected ID" after \System\Model::save() regardless of which action occurred.

Anything else?

I'm stoked contribute to core, but I didn't want to spend the time on a PR if there is no desire to change this behaviour. Please advise.

I can write the update to the docs too if I'm pointed in the right direction to do so.

@evansharp evansharp added the bug Verified issues on the current code behavior or pull requests that will fix them label Oct 22, 2024
@michalsn
Copy link
Member

IMO this may be quite problematic.

The insertID is closely related to the insert query - using the same variable for the update query can be confusing.

Others' code may rely on the fact that insertID is only populated if a new record is added to the table.

So this "small" change may break many things for others.

@michalsn michalsn removed the bug Verified issues on the current code behavior or pull requests that will fix them label Oct 22, 2024
@michalsn michalsn changed the title Bug: Make getInsertID() more consistent with save() Dev: Make getInsertID() more consistent with save() Oct 22, 2024
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