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

Fixed incomplete return type signature on insertGetId method. Updated Model class and added test. #54410

Closed
wants to merge 1 commit into from

Conversation

skoskie
Copy link

@skoskie skoskie commented Jan 30, 2025

Explanation

There can be unexpected consequences when calling methods like $model->insertAndSetId() or $table->insertGetId() are called and the returned value is false, which is not accounted for in the signatures of those methods. This can happen because both functions return the result of Illuminate\Database\Query\Processors\Processor\processInsertGetId() (also needing a PHPDoc update), which in turn returns the result of PDO\lastInsertId(), which correctly indicates it could return a value of false.

Changes

  1. I have updated the PHPDoc signatures in places where it seemed appropriate.
  2. I have updated the Model\insertAndSetId() method to not set the attribute if the return value is false. There must be a better way to handle this, but I wanted to be consistent with other database functions and not throw an error.
  3. I have added a test for the Model class to handle the case in which the attribute is not set. I'm not sure if I put my test in the right place. Improvements are probably needed anyway.

Note

There are many other places where a false return value from lastInsertId() could be problematic. Further review and tests are encouraged.

Beware

  1. This is my first ever attempt at contributing so please accept my apologies if I'm not doing it correctly. 🙏
  2. The change to the Model class seems like a be a breaking change to me, so based on the guidelines, I'm hoping I selected the right branch. Again, sorry if I'm getting this wrong.

…del test.

Note there are many other places where a 'false' return value from lastInsertId() could be problematic. Further review and tests are needed.

Signed-off-by: Shelton Koskie <shelton@eightygrit.com>
@taylorotwell
Copy link
Member

Going to hold off on this for now.

@skoskie
Copy link
Author

skoskie commented Jan 30, 2025

Great. Could you at least accept the PHPdoc updates so that the IDE gives the correct information? If Laravel doesn't handle the possible cases, then it should at least be clear to end developers that they have to account for it. From the indicated return type, it isn't clear at all, leading to preventable errors.

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

Successfully merging this pull request may close these issues.

2 participants