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 setKeysForSelectQuery method and use it when refreshing model data #34974

Merged
merged 1 commit into from Oct 27, 2020
Merged

Conversation

adamthehutt
Copy link
Contributor

@adamthehutt adamthehutt commented Oct 26, 2020

This PR is in reference to #34954.

It adds two methods to the base Model class: setKeysForSelectQuery() and getKeyForSelectQuery(). These methods are then used in fresh() and refresh() instead of the existing setKeysForSaveQuery() and getKeyForSaveQuery().

@GrahamCampbell GrahamCampbell changed the title Add setKeysForSelectQuery method and use it when refreshing model data [8.x] Add setKeysForSelectQuery method and use it when refreshing model data Oct 27, 2020
@GrahamCampbell
Copy link
Member

Thanks for the PR. Isn't this a breaking change?

@adamthehutt
Copy link
Contributor Author

@GrahamCampbell I don't think it is, no. This just addresses a change that was made in #34836. Prior to that, the setKeysForSelectQuery() method was not being called on fresh() or refresh(). That was (for me, anyway) a breaking change. And it seemed semantically incorrect, since refresh() and fresh() are not save queries. @driesvints and @taylorotwell seemed to agree in thread #34954.

@driesvints
Copy link
Member

I think this looks okay but will need @taylorotwell to verify it.

@taylorotwell taylorotwell merged commit 15ee5e6 into laravel:8.x Oct 27, 2020
@axlon
Copy link
Contributor

axlon commented Nov 12, 2020

@GrahamCampbell @adamthehutt @driesvints This is indeed a BC, because this breaks refresh and fresh on pivots and morphpivots again (which my original PR intended to fix). I just composer updated and have tests that are suddenly failing.

The error:

SQLSTATE[42S22]: Column not found: 1054 Unknown column 'id' in 'where clause' (SQL: select * from `invites` where `id` is null limit 1)

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.

None yet

5 participants