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

Show deprecated message instead of raise exception in compiled_method_container method #35281

Conversation

y-yagi
Copy link
Member

@y-yagi y-yagi commented Feb 15, 2019

Since #35036, the subclasses of ActionView::Base requires the compiled_method_container method.
This is incompatible. For example, web-console use view class that subclass of ActionView::Base, and does not work it now cause of this.

I am not sure that using self.class is correct, but I think that it is better to show a deprecate message rather than raise exception.

@rails-bot rails-bot bot added the actionview label Feb 15, 2019
@y-yagi
Copy link
Member Author

y-yagi commented Feb 16, 2019

cc @tenderlove

@tenderlove
Copy link
Member

I think it can break (or leak memory) in some cases, but I think you're right: it's better to emit a warning and continue to work than to raise an exception.

Web console probably doesn't get reloaded, so there's probably no memory leak.

Actually, maybe we should only emit the warning if self.class == AV::Base?

…d_container` method

Since rails#35036, the subclasses of `ActionView::Base` requires
the `compiled_method_container`.
This is incompatible. For example, `web-console` use view class that
subclass of `ActionView::Base`, and does not work it now cause of this.

Actually, since it seems that it is only `ActionView::Base` that
`compiled_method_container` is necessary, modified the condition that
emits a warning.
@y-yagi y-yagi force-pushed the show_deprecated_message_instead_of_raise_exception branch from 2960d4f to 7878027 Compare February 16, 2019 04:04
@y-yagi
Copy link
Member Author

y-yagi commented Feb 16, 2019

Actually, maybe we should only emit the warning if self.class == AV::Base?

I think that it makes sense because I couldn't come up with a case with an original view class other than debug view. I fixed condition and message.

@tenderlove tenderlove merged commit fdc0595 into rails:master Feb 16, 2019
@y-yagi y-yagi deleted the show_deprecated_message_instead_of_raise_exception branch February 16, 2019 07:35
vipulnsward added a commit to vipulnsward/rails that referenced this pull request Mar 12, 2019
…led container.

Use the suggested with_empty_template_cache to overcome warning
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants