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

[8.x] Add firstOr() function to BelongsToMany relation #40828

Merged
merged 2 commits into from
Feb 11, 2022

Conversation

r-kujawa
Copy link

@r-kujawa r-kujawa commented Feb 6, 2022

What does this PR do?

  • It adds the firstOr() function to the BelongsToMany::class relationship.
  • Adds tests to support this addition ☝🏼.

Why do we need this?

Before this PR

The 'firstOr()' function was being executed via the __call() method of the BelongsToMany::class which forwarded the call to the Eloquent\Builder::class. When the firstOr() function found a first() record it returned the relationship instance, but didn't add the necessary select columns ($this->shouldSelect($columns)) to the query builder object.

SELECT * 
FROM `posts` 
INNER JOIN `users_posts` ON `posts`.`uuid` = `users_posts`.`post_uuid` 
WHERE `users_posts`.`user_uuid` = ? 
ORDER BY RAND() 
LIMIT 1;

With this PR

By adding the firstOr() function to the BelongsToMany::class itself, the get() that executes the select query statement is now the one that lives in the BelongsToMany::class and defines the columns that should be selected.

SELECT `posts`.*, `users_posts`.`user_uuid` AS `pivot_user_uuid`, `users_posts`.`post_uuid` AS `pivot_post_uuid`, `users_posts`.`is_draft` AS `pivot_is_draft`, `users_posts`.`created_at` AS `pivot_created_at`, `users_posts`.`updated_at` AS `pivot_updated_at` 
FROM `posts` 
INNER JOIN `users_posts` ON `posts`.`uuid` = `users_posts`.`post_uuid` 
WHERE `users_posts`.`user_uuid` = ? 
ORDER BY RAND() 
LIMIT 1;

Thanks to the get() function being executed directly on the BelongsToMany::class's relation, it adds the select columns, and prevents the Post::class's model from inheriting the wrong id and/or any other pivot table (users_posts) column with the same name as any of the columns in the posts table.

@r-kujawa r-kujawa marked this pull request as ready for review February 6, 2022 14:38
@driesvints
Copy link
Member

Please send this to 9.x

@driesvints driesvints closed this Feb 7, 2022
@r-kujawa
Copy link
Author

r-kujawa commented Feb 7, 2022

Please send this to 9.x

I could do that, but won't 8.x be receiving bug fixes anymore?

@driesvints
Copy link
Member

@r-kujawa this is a new feature, not a bug fix?

@r-kujawa
Copy link
Author

r-kujawa commented Feb 7, 2022

@driesvints I believe it is a bug fix, because if you call the firstOr() method on a belongsToMany() relation it will not return exactly what is expected if the pivot table has a column of the same name as the requested relations column.

e.g. If the users_posts table has an id column (or timestamps() columns) when retrieving the first() record of the database, the model retrieved will inherit the values from the pivot table columns instead of the actual models value. You may reference the test I wrote in this PR to validate this.

$post = $user->posts()->firstOr(function () {
    // return default post...
});

If the pivot table in the above query has an id of 101, but the first() post being retrieved has an id of 49, it will return the actual post, but the id will be 101 instead of the expected 49. This is because of the INNER JOIN statement without specifying the columns to be selected from the query.

@r-kujawa
Copy link
Author

r-kujawa commented Feb 7, 2022

Here is a reference from somebody else experiencing this issue, hope it helps.

@driesvints
Copy link
Member

@r-kujawa you're correct! Sorry about that.

@driesvints driesvints reopened this Feb 7, 2022
@driesvints driesvints changed the title Add firstOr() function to BelongsToMany relation [8.x] Add firstOr() function to BelongsToMany relation Feb 7, 2022
@r-kujawa
Copy link
Author

r-kujawa commented Feb 7, 2022

@r-kujawa you're correct! Sorry about that.

No worries, always happy to help 😄

@taylorotwell
Copy link
Member

This was actually already on 9.x back in September.

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