-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
feat: add method to insert empty data in Model #6109
feat: add method to insert empty data in Model #6109
Conversation
c1ad5d5
to
9cfcfe3
Compare
It seems OCI8 driver can't get the insert id.
https://github.com/codeigniter4/CodeIgniter4/runs/6840259577?check_suite_focus=true |
I saw on a forum or a github issue that someone came across the inability to use an empty request. The solution is to use a raw query or add a tinyint(1) column with arbitrary data. |
It makes sense if it has links to other tables. |
I wrote about links with other tables. You either add data or you don't. |
It is not to add a record without data. The record will have ID. The point is sometimes we need this functionality to avoid "no data to insert" exception of |
The table consisting of (id, created_at, updated_at) seems strange to me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this feature. Some implementation suggestions and requests.
system/BaseModel.php
Outdated
/** | ||
* Whether to prohibit inserting empty data. | ||
*/ | ||
protected bool $prohibitInsertEmpty = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider naming after the existing properties.
protected bool $prohibitInsertEmpty = true; | |
protected bool $allowEmptyInserts = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
The table could have any amount of data from default values. If a developer wants to let the database handle those, as it stands currently you need to submit some "dummy" data. I think our protections in general are good but I am a fan of the option to override those protections when needed. Example based on kenjis:
This fails:
|
@ytetsuro Can you debug this? It seems only Oracle can't get the insert id. |
Ok! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a58dc17
to
6729351
Compare
@ytetsuro Thank you so much! |
I would like to hear if @iRedds has any other thoughts before we move on from the discussion. |
I think if we support insertion of empty data, Query Builder should support it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
@kenjis Your diff link doesn't work for me on mobile so I'm not sure what the issue is in Query Builder, but I am fine with adding a parallel feature there. |
@MGatner Sorry, I didn't explain what the issue is in Query Builder. Query Builder also prohibit inserting no data. |
6729351
to
6ab4d62
Compare
Rebased and added the documentation. @iRedds Any comments? |
My only thought on the docs is maybe to mention an example of why one might want do this. |
ErrorException : mb_strpos(): Passing null to parameter #1 ($haystack) of type string is deprecated .../CodeIgniter4/system/Validation/FileRules.php:138
This is an important config change.
Now allowEmptyInserts() is a normal setter.
Co-authored-by: ytetsuro <[email protected]>
6ab4d62
to
a48cba9
Compare
Resolved conflict. |
Sorry, I made a mistake when I rebased. I mistakenly rebased with |
Shouldn't be a problem, |
Okay, thanks.
No, it is daily job. If we do not merge, bugs will remain unfixed in |
If that becomes tiresome we could set up a GitHub Action to merge to 4.3 anytime develop is updated. I'd be willing to look into that. |
Description
Model::allowEmptyInserts()
to insert empty dataChecklist: