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

Different behavior of html_safe in dev/prod #305

Open
ilya-konanykhin opened this issue Jun 26, 2015 · 20 comments
Open

Different behavior of html_safe in dev/prod #305

ilya-konanykhin opened this issue Jun 26, 2015 · 20 comments

Comments

@ilya-konanykhin
Copy link

In production & staging environments the result of cell rendering is returned as escaped HTML string. In order to get raw result (as excepted) I have to do cell(...).call:

= cell :welcome_stats, :some_stuff # this renders <div>...
= cell(:welcome_stats, :some_stuff).call # this works as expected: <div>...

Dev env works well.

Gem versions:
rails 4.2.2
cells 4.0.1
cells-slim 0.0.3

Here's config:

config.cache_classes = true
config.eager_load = true

config.consider_all_requests_local       = false
config.action_controller.perform_caching = true

config.assets.js_compressor = :uglifier
config.assets.compile = false
config.assets.digest = true
config.serve_static_files = ENV['RAILS_SERVE_STATIC_FILES'].present?
config.action_dispatch.x_sendfile_header = 'X-Accel-Redirect'

config.log_level = :info

config.i18n.fallbacks = true

config.active_support.deprecation = :notify

config.log_formatter = ::Logger::Formatter.new

config.action_controller.default_url_options = {host: "..."}
config.action_mailer.default_url_options = {host: "..."}
config.action_mailer.delivery_method = :file

config.neofiles.cdns = ['...', '...']

Any suggestions?

@apotonick
Copy link
Member

Not sure I understand the problem? ViewModel#call will html_safe the content in Rails, calling the method manually won't. Is that poorly documented?

@apotonick
Copy link
Member

This is documented here: https://github.com/apotonick/cells#invocation-styles

Calling the method manually is not official API and up to you. This is also discussed at the end of chapter 8 in the Trailblazer book, where you can call states manually and avoid Capybara wrapping.

@ilya-konanykhin ilya-konanykhin changed the title Cached content doesn't get html_safed Different behavior of html_safe in dev/prod Jun 27, 2015
@ilya-konanykhin
Copy link
Author

Uh-oh, excuse me. I changed title of this issue (I realized the real problem while submitting and re-wrote text, but forgot about title).

Now, the problem is different behavior in environments: when I was in dev and used cell :name, :model (without .call) I got what I wanted, HTML was rendered properly (html_safed). When I uploaded that code without change to staging, it started escaping HTML entities (no html_safe).

Maybe, in staging & production module Cell::RailsExtensions::ViewModel doesn't get mixed in for some reason? I deploy code via Capistrano and restart server with each deploy, trying puma restart and puma start/puma stop. Nothing helped until I added .call, now it works consistently in all environments.

@apotonick
Copy link
Member

Ah ok, thanks, now I understand your pain.

I can't say it for sure, but if Cell::RailsExtensions::ViewModel did not get included properly, many other things wouldn't work, either. Also, you said "when using .call it works. That also implies the module got mixed in and html_safes the return value of call.

Can you confirm that? When you do cell(..).call it works in both environments?

@apotonick apotonick reopened this Jun 27, 2015
@ilya-konanykhin
Copy link
Author

Just made a clean check — rendered the same cell subsequently with .call and without it, with .call works well in both environments, without it in staging produces escaped output. So yes, it is confirmed.

I observe this behavior on cells both with and without caching, don't think it is related.

Extensions inclusion: I see your point and made a little investigation, with no luck though. I'll try digging deeper.

@firedev
Copy link
Contributor

firedev commented Sep 29, 2015

I concur. This looks strange in the view:

= cell('worktime', worktime).()

Shouldn't view helper return html_safe output by default?

Besides

= cell('worktime', collection: worktimes)

Returns an html_safe string.

@apotonick
Copy link
Member

That's right, in views, cell(..) will result in cell(..).to_s will result in cell(..).(:show) will result in a html_safe string.

@firedev
Copy link
Contributor

firedev commented Sep 29, 2015

Yep, I understand, but if we are striving for consistency then maybe adding something like this to base class would be reasonable?

def to_s
  call.html_safe
end

@apotonick
Copy link
Member

..which is exactly what happens: https://github.com/apotonick/cells/blob/master/lib/cell/rails.rb#L47

I just messages you on IRC to please join Trailblazer gitter channel.

@siegfried
Copy link

@apotonick Any news about this issue? It has blocked our project. What can I help?

@apotonick
Copy link
Member

I have zero clue what might be wrong, as it works for me in many projects. Do you have a minimal cell that can provoke this error? Or does it happen with nested cells?

@siegfried
Copy link

It only occurs to SLIM in production. In development it is fine. And calling cell/concept with collection would be fine as well. It seems that this problem does not happen to HAML anymore. @apotonick

@siegfried
Copy link

I think I got it fixed by using '==' instead of '='. Maybe that is the correct way to use cells in slim. However, it is just strange that development and production have different behaviours.

@szyablitsky
Copy link

+1 for this problem in slim.
Also have to use == instead of = in slim templates.

@apotonick
Copy link
Member

So, should we simply document that in cells-slim? I have no clue how to fix that and zero time to investigate. It must be something with this bloody html_safe string marking?

@ilya-konanykhin
Copy link
Author

ilya-konanykhin commented Sep 20, 2016

I found this old issue and felt it would be nice if it can be finally resolved.

_TL/DR_: add to your cells a method html_safe? returning true.

In details, it seems the problem is in Slim engine's test for html_safe? before rendering anything:

= cell(:test)
# ERB equivalent: <%= ::Temple::Utils.escape_html_safe((cell(:test))) %>

The method escape_html_safe is like this:

def escape_html_safe(html)
  html.html_safe? ? html : escape_html(html)
end

(This method may or may not be called depending on a combination of settings — I guess that's the reason why some folks see different behavior in dev/prod, including me.)

This leads to:

= cell(:test).html_safe?      # false
= cell(:test).to_s.html_safe? # true
= cell(:test).call.html_safe? # true

As we really do cell html_safing in Rails, it seems OK to add its sister method somewhere near:

# lib/cell/rails.rb
def call(*)
  super.html_safe
end

def html_safe?
  true
end

This was tested in cells 4.0.5 and cells 4.1.3 + cells-rails 0.0.6.

What do you think, can we add this? I can make a pull request if you give it a go.

@apotonick
Copy link
Member

apotonick commented Oct 15, 2016

Yes, this doesn't make sense to me, but it makes sense in Rails. 🤔 How could we test this?

This must go to the cells-rails gem, though.

@ilya-konanykhin
Copy link
Author

I did fresh installation of latest Rails and Slim, and tested both with old cells (4.0.5) and new cells + cells-rails (4.1.3 + 0.0.6). That is, I added method html_safe? returning true to lib/cell/rails.rb and all worked well, resolving the issue.

Would this suffice? If not, can you suggest other tests to do?

@ilya-konanykhin
Copy link
Author

I've made a pull request — take a look at it when you have a chance: https://github.com/trailblazer/cells-rails/pull/12

@phillipoertel
Copy link

I found this old issue and felt it would be nice if it can be finally resolved.

_TL/DR_: add to your cells a method html_safe? returning true.

The PR is gone. Creating a new initializer (like config/initializers/cells_html_escaping_patch.rb) and putting Ilja's suggestion in there fixed the issue for me--thank you 💪

module Cell
  module RailsExtensions
    module ViewModel
      def html_safe?
        true
      end
    end
  end
end

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

No branches or pull requests

6 participants