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] Fixed incorrect assumption in QueriesRelationships that the owner key is necessarily the model key #50706

Closed

Conversation

GrahamCampbell
Copy link
Member

@GrahamCampbell GrahamCampbell commented Mar 21, 2024

Fixes a bug introduced earlier (#38668) that nobody noticed because:

  1. Using a whereMorphedTo is a pretty rare use case. I've never used it before, and neither does any of the first party library code that I have locally.
  2. Almost all of the time the owner id will be the same as the related model id, as that's the default and it's unusual to change it.

@GrahamCampbell GrahamCampbell changed the title Fixed incorrect assumption in QueriesRelationships that the owner k… [10.x] Fixed incorrect assumption in QueriesRelationships that the owner key is necessarily the model key Mar 21, 2024
@GrahamCampbell
Copy link
Member Author

cc @tobyzerner

@GrahamCampbell
Copy link
Member Author

Ok, I'm not sure why the tests are failing here. getRelatedKeyFrom just runs $model->{$this->ownerKey} which is the same as what we had before in $model->getKey() whenever the key column and owner column are the same, and they both are set to id in the tests?

@tobyzerner
Copy link
Contributor

Good catch @GrahamCampbell! Not sure why the tests are failing either, strange 🤔

@staudenmeir
Copy link
Contributor

getRelatedKeyFrom() is a protected method and so the call is handled by Relation::__call() which throws a BadMethodCallException exception (that somehow doesn't show up).

@GrahamCampbell
Copy link
Member Author

GrahamCampbell commented Mar 23, 2024

Thanks. My 10:30PM brain didn't even think to check that the method was not public. 😆

@taylorotwell
Copy link
Member

Probably need a test for this.

@taylorotwell taylorotwell marked this pull request as draft March 24, 2024 15:08
@driesvints
Copy link
Member

@GrahamCampbell feel free to resend this with a test, thanks.

@driesvints driesvints closed this Apr 11, 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.

5 participants