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

Related data belonging to another parent should not be updated #7

Closed
wants to merge 2 commits into from

Conversation

bianchi
Copy link
Contributor

@bianchi bianchi commented Aug 1, 2023

When the primary key of the related data did not belong to parent, change was still being made.

Eg:

$user1->tasks()->sync([
            // Update
            [
                'id' => 3, // id from a task which belongs to another user
                'content' => 'Trying to change a task which is not mine',
            ],
 ]);

I see two approaches:

  1. Ignore this record. In the example user1 would end with no tasks
  2. Creates a new record. In the example user1 would end with a new task with another id
  3. Throws an error

I implemented option 2 here.

@korridor korridor self-assigned this Sep 7, 2023
@korridor
Copy link
Owner

korridor commented Sep 7, 2023

@bianchi Thanks for the PR, and thank you for your patience! For me, this was not a problem since we validate the array that we put in the sync() function, but I agree that this is unexpected behavior and can lead to issues.

I will look into this in more detail the next week!

@korridor
Copy link
Owner

korridor commented Mar 1, 2024

@bianchi Sorry I forgot about the issue. I resolved the issue you reported here in the PR #10.

I implemented the approach 3 and there is now a new function attribute called $throwOnIdNotInScope that can be set to false to get the behavior of approach 1.

The reason I did not use your implementation of approach 2 is that I could not think of a scenario in which this would be my preferred behavior. If you provide me with a real-world scenario in which approach 2 is needed, I might reconsider.

Since you reported this bug, I'll, of course, add you to the credits of the release!

@korridor korridor closed this Mar 1, 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

Successfully merging this pull request may close these issues.

2 participants