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

Introduce `reload_<association>` reader for singular associations. #27133

Merged
merged 1 commit into from Nov 22, 2016

Conversation

Projects
None yet
6 participants
@senny
Member

senny commented Nov 21, 2016

Summary

This patch brings back the functionality of passing true to the
association proxy. The behavior was deprecated with #20888 and scheduled
for removal in Rails 5.1.

The deprecation mentioned that instead of Article.category(true) one
should use article#reload.category. Unfortunately the alternative does
not expose the same behavior as passing true to the reader
did. Specifically reloading the parent record throws unsaved changes and
other caches away. Passing true only affected the association.

This is problematic and there is no easy workaround. I propose to bring
back the old functionality by introducing this new reader method for
singular associations.

Implications

We started to issue deprecation warnings for this since 5.0.0. If this is merged, I think we should backport the patch to 5-0-stable and update the deprecation warning accordingly.

@senny

This comment has been minimized.

Member

senny commented Nov 21, 2016

@dhh we discussed this some time ago. If possible I'd like to bring this back to 5.0.x and update the deprecation warnings. The current suggestion to change to article.reload.category can cause bugs that are hard to track down.

/cc @matthewd

@dhh

This comment has been minimized.

Member

dhh commented Nov 21, 2016

I thought we had agreed on using #reload_category or another dynamic method? I don't like using a mystery boolean like that. It's a terrible API (even if I originally designed it!).

@kaspth

This comment has been minimized.

Member

kaspth commented Nov 21, 2016

@dhh that is what Yves is proposing in this PR. 😊

@dhh

This comment has been minimized.

Member

dhh commented Nov 21, 2016

Ha! I should have read the diff, not the description. Silly me. 👍

@bogdan

This comment has been minimized.

Contributor

bogdan commented Nov 22, 2016

The force reloading of the association reader was deprecated in #20888.

Sounds very sad.

@dhh dhh removed the needs feedback label Nov 22, 2016

Introduce `reload_<association>` reader for singular associations.
This patch brings back the functionality of passing true to the
association proxy. The behavior was deprecated with #20888 and scheduled
for removal in Rails 5.1.

The deprecation mentioned that instead of `Article.category(true)` one
should use `article#reload.category`. Unfortunately the alternative does
not expose the same behavior as passing true to the reader
did. Specifically reloading the parent record throws unsaved changes and
other caches away. Passing true only affected the association.

This is problematic and there is no easy workaround. I propose to bring
back the old functionality by introducing this new reader method for
singular associations.

@senny senny force-pushed the reload_singular_associations branch from 21f9a6e to 0e99571 Nov 22, 2016

@senny senny merged commit 44087d8 into master Nov 22, 2016

3 checks passed

codeclimate Code Climate didn't find any new or fixed issues.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@senny senny deleted the reload_singular_associations branch Nov 22, 2016

@kaspth

This comment has been minimized.

Member

kaspth commented Nov 22, 2016

I see the deprecation warning in the reader method still says passing true is deprecated, but recommends calling reload on the parent object.

We should probably change that wording now :)

@senny

This comment has been minimized.

Member

senny commented Nov 22, 2016

@kaspth I'm on it. I guess for master the message will just be removed along with the possibility to pass true. Or am I mistaken? My backport to 5-0-stable will change the wording.

@kaspth

This comment has been minimized.

Member

kaspth commented Nov 22, 2016

I guess for master the message will just be removed along with the possibility to pass true.

👍. But how will 5.0 users who have replaced the category(true) instances with reload.category then see our changed recommendation? A mention in the 5.1 release notes?

My backport to 5-0-stable will change the wording.

👍. We should probably update the release notes as well.

senny added a commit that referenced this pull request Nov 22, 2016

Merge pull request #27133 from rails/reload_singular_associations
Introduce `reload_<association>` reader for singular associations.

This backport contains modifications to the deprecation warning. This
will smoothen the upgrade path for users that were relying on force
reloading singular associations.
@senny

This comment has been minimized.

Member

senny commented Nov 22, 2016

Backported with ecb394a

@matthewd

This comment has been minimized.

Member

matthewd commented Nov 22, 2016

Anyone successfully using the old recommendation is fine. It's only insufficient for a narrow use case.

(Do we want to recommend both? Or is that just confusing?)

@senny

This comment has been minimized.

Member

senny commented Nov 22, 2016

@matthewd I don't think it's necessary. Honestly now that we have reload_article I would recommend everyone who was passing true to use that. Reloading the parent has many more side effects and is simply not needed when all you want is the latest version of the association.

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