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

`reload` should fully reload attributes #15866

Merged
merged 1 commit into from Jun 22, 2014
Merged

Conversation

@sgrif
Copy link
Contributor

sgrif commented Jun 22, 2014

reload is meant to put a record in the same state it would be if you
were to do Post.find(post.id). This means we should fully replace the
attributes hash.

`reload` is meant to put a record in the same state it would be if you
were to do `Post.find(post.id)`. This means we should fully replace the
attributes hash.
senny added a commit that referenced this pull request Jun 22, 2014
`reload` should fully reload attributes
@senny senny merged commit 7035a20 into rails:master Jun 22, 2014
1 check was pending
1 check was pending
continuous-integration/travis-ci The Travis CI build is in progress
Details
@sgrif sgrif deleted the sgrif:sg-reload-attributes branch Jun 22, 2014
senny added a commit that referenced this pull request Jun 22, 2014
/cc @sgrif
@senny
Copy link
Member

senny commented Jun 22, 2014

This introduces a slight backwards incompatibility. You were able to access attributes from a custom select after a reload. Those attributes won't change after the reload so I look at this as a bug fix.

It would be nice to issue a deprecation in these cases but I don't think we can easily detect wether someone is accessing these attributes.

/cc @rafaelfranca

@rafaelfranca
Copy link
Member

rafaelfranca commented Jun 26, 2014

hmm, not sure about this change. I'd expect the select attributes to be there after the reload.

@senny
Copy link
Member

senny commented Jun 26, 2014

but you would also expect, that they were reloaded, no?

@rafaelfranca
Copy link
Member

rafaelfranca commented Jun 26, 2014

Ah, yes. If they are not being reloaded so it make sense to remove this behavior.

@senny
Copy link
Member

senny commented Jun 26, 2014

@rafaelfranca exactly. Removing them is not perfect but it's better than keeping them around without updating them. This could lead to bad bugs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.