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] Fix pivot and morphpivot fresh and refresh methods #35193

Merged
merged 2 commits into from
Nov 13, 2020
Merged

[8.x] Fix pivot and morphpivot fresh and refresh methods #35193

merged 2 commits into from
Nov 13, 2020

Conversation

axlon
Copy link
Contributor

@axlon axlon commented Nov 12, 2020

This PR fixes Pivot::fresh(), Pivot::refresh(), MorphPivot::fresh() and MorphPivot::refresh() which are broken after #34974 which reverted #34836.

I have tested this locally already, but I can add additional tests if these changes are okay.

@driesvints
Copy link
Member

driesvints commented Nov 12, 2020

Can you add a test for this? Also: doesn't this re-introduces #34954?

/cc @adamthehutt

@axlon
Copy link
Contributor Author

axlon commented Nov 12, 2020

If I understood his comments correctly he extended Model::setKeysForSaveQuery() which was being called by Model::fresh() and Model::refresh() after my first PR.

His PR then changed this to the newly introduced Model::setKeysForSelectQuery() so that his project wouldn't break anymore (because his extended function is no longer called when it shouldn't be), which would have been fine, but the PR did not account for Pivot::fresh(), Pivot::refresh(), MorphPivot::fresh() and MorphPivot::refresh().

All this PR aims to do is fix the pivot methods which were broken by aforementioned PR, not revert his PR altogether so it shouldn't re-introduce the original issue (unless I misunderstood the change that was made)

@driesvints I will add tests in a few hours when I have some free time 👍

@axlon
Copy link
Contributor Author

axlon commented Nov 12, 2020

@driesvints I added tests

@adamthehutt
Copy link
Contributor

Sorry about that, @axlon.

@driesvints It looks like this should be fine as far as I can tell.

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.

4 participants