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

[12.x] Allow fillAndInsert() usage with HasMany relationships by automatically setting the foreign key #55288

Draft
wants to merge 4 commits into
base: 12.x
Choose a base branch
from

Conversation

KentarouTakeda
Copy link
Contributor

This pull request is a follow-up to #55038 introduced in v12.6.0 by @cosmastech. While fillAndInsert() was successfully added to allow merging attributes before insertion, it can cause errors when used on a HasMany relationship if the related table enforces a NOT NULL constraint on the foreign key.

For instance, the following snippet will fail on certain table definitions:

  • Repro code
    $user->posts()->fillAndInsert([
        [ /* omission */ ]
    ]);
  • Error message
    Integrity constraint violation: 19
      NOT NULL constraint failed: posts_having_uuids.user_id
    
  • SQL
    insert into "posts_having_uuids"
      ("created_at", "id", "name", "published_at", "status", "status_string", "updated_at", "uuid")
    values
      /* omission */

With the current implementation (without this PR), the user_id column is not automatically populated, resulting in a constraint violation. This PR ensures that fillAndInsert() can correctly handle the foreign key column for HasMany relationships, preventing such errors.

(Above snippet and error details are included within the tests to demonstrate the exact failure under real conditions.)

@KentarouTakeda KentarouTakeda force-pushed the fill-and-insert-into-has-many-relationship branch from 675f562 to 80bbca4 Compare April 6, 2025 03:07
@taylorotwell
Copy link
Member

I'll need @cosmastech's opinion, but I'm not certain this method was ever meant to be used with relationships.

@taylorotwell taylorotwell marked this pull request as draft April 7, 2025 21:14
@cosmastech
Copy link
Contributor

cosmastech commented Apr 7, 2025

I'll need @cosmastech's opinion, but I'm not certain this method was ever meant to be used with relationships.

@taylorotwell I see no reason not to extend it to relationships. I believe it currently would be available, just wouldn't work as a user might expect, since it's not explicitly setting the relationship values. (edit: insert() also doesn't really work with setting the default relationship values 🤷, so maybe that could be fixed too?)

I haven't tested this myself, so I can't confirm the functionality, but conceptually I think it makes sense.

@KentarouTakeda
Copy link
Contributor Author

@cosmastech
It's easy to add to insert(), but I think it should be considered carefully for the following reasons:

  • insert() without fill will automatically fill the parent_id
  • It will change behavior that has existed for a long time

In my opinion, it would be better to postpone the fix in minor versions or discuss it in another forum.

@cosmastech
Copy link
Contributor

@cosmastech It's easy to add to insert(), but I think it should be considered carefully for the following reasons:

  • insert() without fill will automatically fill the parent_id
  • It will change behavior that has existed for a long time

In my opinion, it would be better to postpone the fix in minor versions or discuss it in another forum.

@KentarouTakeda makes sense to me. 👍

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.

3 participants