Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

PHPORM-127 Fix priority of embeds vs attributes #2584

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

Open
GromNaN wants to merge 1 commit into mongodb:5.x
base: 5.x
Choose a base branch
Loading
from GromNaN:embeds-priority

Conversation

Copy link
Member

@GromNaN GromNaN commented Aug 29, 2023 •
edited
Loading

Fix #2577 (continuing)
Fix PHPORM-127

Moving the code from the Model class to the EmbedsRelations trait.

@GromNaN GromNaN force-pushed the embeds-priority branch 2 times, most recently from da4339c to 81a0fc6 Compare August 30, 2023 12:37
$instance = new $related;

if ($otherKey === $relation) {
throw new \LogicException(sprintf('In %s::%s(), the key cannot be identical to the relation name "%s". The default key is "%s".', static::class, $relation, $relation, $instance->getForeignKey().'s'));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mshamaseen wrote:

The solution was so easy, just don't name the foreign key the same as the relation; that is the default behavior if you don't pass any foreign keys to $this->belongsToMany(related , /* Don't pass anything else */) , and that is also the documented way to do it.

I propose to throw an exception in this case to avoid any error.

mshamaseen and jmikola reacted with thumbs up emoji

public function otherGroups()
{
return $this->belongsToMany(Group::class, 'groups', 'users', 'otherGroups', '_id', '_id', 'groups');
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to add a test for the exception.

@GromNaN GromNaN added this to the 4.0.0 milestone Aug 30, 2023
@GromNaN GromNaN reopened this Dec 19, 2023
@GromNaN GromNaN changed the title (削除) Fix priority of embeds vs attributes (削除ここまで) (追記) PHPORM-127 Fix priority of embeds vs attributes (追記ここまで) Dec 19, 2023
@alcaeus alcaeus changed the base branch from 4.1 to 5.x September 12, 2024 08:53
@alcaeus alcaeus requested a review from a team as a code owner September 12, 2024 08:53
@alcaeus alcaeus requested review from jmikola and removed request for a team September 12, 2024 08:53
}
}
```
**Warning:** naming the foreign key same as the relation name will prevent the relation for being called on dynamic property, i.e. in the example above if you replaced `group_ids` with `groups` calling `$user->groups` will return the column instead of the relation.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think "e.g." would be more appropriate because you're providing one possible example.

See: https://www.merriam-webster.com/grammar/ie-vs-eg-abbreviation-meaning-usage-difference


**REMEMBER**: These relations return Eloquent collections, they don't return query builder objects!

**Breaking changes** starting from v4.0 you need to define the return type of EmbedsOne and EmbedsMany relation for it to work
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see this PR targets 5.x. Should this documentation change be backported?

Also, is this heading better written as:

**Breaking Change:** starting from...

You're only noting a single breaking change, and it appears to be a label (vs. part of the sentence) so a colon could help differentiate. This applies below as well.

*/
private function hasEmbeddedRelation(string $key): bool
{
if (! method_exists($this, $method = Str::camel($key))) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've never bothered to do this myself, but assumed CS for PHPLIB prohibited such inline assignments in the interest of readability. Is this common within the Laravel package?

Happy to defer to you.

$instance = new $related;

if ($relatedPivotKey === $relation) {
throw new \LogicException(sprintf('In %s::%s(), the key cannot be identical to the relation name "%s". The default key is "%s".', static::class, $relation, $relation, $instance->getForeignKey().'s'));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can optionally use argument numbering to remove the duplicate $relation argument.

Suggested change
throw new \LogicException(sprintf('In %s::%s(), the key cannot be identical to the relation name "%s". The default key is "%s".', static::class, $relation, $relation, $instance->getForeignKey().'s'));
throw new \LogicException(sprintf('In %1$s::%2$s(), the key cannot be identical to the relation name "%2$s". The default key is "%3$s".', static::class, $relation, $instance->getForeignKey().'s'));

if (
method_exists($this, $key)
&& ! method_exists(self::class, $key)
&& ! $this->hasAttributeGetMutator($key)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what this method does, but it seems the fix here is replace this with checking that the method returns either EmbedsOne or EmbedsMany (per your EmbedsRelations trait).

public function users(): BelongsToMany
{
return $this->belongsToMany(User::class, 'users', 'groups', 'users', '_id', '_id', 'users');
return $this->belongsToMany(User::class, 'users', 'groupIds', 'userIds', '_id', '_id', 'users');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is renaming to "userIds" the meaningful change here, so that it doesn't conflict with the relationship name? Is the renaming of "groupIds" just for consistency?

Copy link
Member

@jmikola jmikola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some optional suggestions and questions. If everything is easily resolved/answered, feel free to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmikola jmikola jmikola approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
4.0
Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /