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

[10.x] Added eachById and chunkByIdDesc to BelongsToMany #50991

Merged
merged 3 commits into from
Apr 10, 2024

Conversation

lonnylot
Copy link
Contributor

@lonnylot lonnylot commented Apr 10, 2024

If you have a BelongsToMany pivot with a incrementing key then you can get the wrong id hydrated in the model and further operations would be on the wrong model.

Similar to #47479, but I think the incrementing key needs to be emphasized. People could be selecting in the current release and getting the wrong ID.

People are adding methods to BuildsQuery and not following through with maintaining BelongsToMany or HasManyThrough. Example: chunkByIdDesc was added a few months ago and nobody even thought about BelongsToMany #48666.

I'd ask internals to consider a larger rewrite as this doesn't appear to be well managed. I could keep writing failing tests (e.g. lazyByIdDesc or orderedLazyById), but I don't have time tonight. Additionally I'm guessing HasManyThrough (or anything else with a pivot) will have similar issues.

@lonnylot lonnylot changed the base branch from 11.x to 10.x April 10, 2024 03:32
@lonnylot
Copy link
Contributor Author

lonnylot commented Apr 10, 2024

I'd ask internals to consider a larger rewrite as this doesn't appear to be well maintained.

I want to emphasize this even more and share that this bug has caused real world harm to my organization.

Someone in my organization, either confused by debugging Eloquent (beautiful software 🙇 , not the easiest to debug 🤗 ) or thinking primary keys on pivots weren't supported, wrote this code :

$relationship = $model->relationship();

$relationship->eachById(
      callback: fn ($relatedModel) => $relatedModel->delete(),
      column: $relationship->getQualifiedRelatedKeyName(),
      alias: 'id',
);

Because $relatedModel has the wrong primary ID we've been deleting the wrong rows and only just noticed now.

I cannot explain why this was done because it was done prior to me starting by someone no longer at the organization.

@taylorotwell taylorotwell merged commit b905597 into laravel:10.x Apr 10, 2024
23 of 24 checks passed
@taylorotwell
Copy link
Member

taylorotwell commented Apr 10, 2024

Thanks 👍 I've fixed chunkByIdDesc on HasManyThrough as well, and lazyByIdDesc on both BelongsToMany and HasManyThrough

@lonnylot
Copy link
Contributor Author

@taylorotwell Thank you 🙏

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