Shopify / liquid

Liquid markup language. Safe, customer facing template language for flexible web apps.
https://shopify.github.io/liquid/
MIT License
11.03k stars 1.38k forks source link

Custom Liquid::Block tag not working properly inside a for loop tag #698

Open passalini opened 8 years ago

passalini commented 8 years ago

Guys, I'm working with a code that gave me an strange behaviour. For example, I have a liquid's post view that renders many comments and its edit's forms, those comments are inside a snippet with a custom tag form.

In this custom tag, the code initialize the @form_type that is used to define the form's path and other things.

Now, the strange thing! This tag works properly outside the for loop, but in the for the initialize function is called just once and the render function is called correctly (x loop times). The problem with it is that in the second time all instance variables defined in the custom tag are nil. With this behaviour the form_by_type function does not return anything and the final html form will be without action, class, id.....

Let me show you some example code:

<!-- post.liquid  -->
{% for comment in post.comments %}
   {% include 'post/comment', c: comment %}
{% endfor%}
<!-- snippets/post/comment.liquid  -->

<div id="comment-{{c.id}}">
  <div class="comment-text">
    {{ collaborative_comment.text }}
  </div>

  <div class="edit-comment hide">
    {% form 'edit_comment', remote: true %}
      <textarea rows="10" cols="80" name="comment[text]">{{c.text}}</textarea>
      <input type="submit" value="Update">
    {% endform %}
  </div>
</div>
# tags/form_tag.rb

class FormTag < Liquid::Block
  include RenderView

  def initialize(tag_name, markup, options)
    super

    @form_html_options = {}
    @form_type         = markup.split(',')[0].strip

    markup.scan(Liquid::TagAttributes) do |key, value|
      @form_html_options[key.to_sym] = value
    end
  end

  def render(context)
    @context    = context
    @form_type  = @context[@form_type]
    @form       = initialize_form
    @resource   = @context[@form[:resource_key]]
    @form_token = @context['authenticity_token']

    @context.stack do
      @context['form'] = {
        'notice' => @context['notice'],
        'errors' => @resource ? @resource['errors'] : @context['alert']
      }

      @html_block = render_all(@nodelist, @context)
    end

    render_view('form')
  end

  private

  def initialize_form
    resource_key, url, method = form_by_type

    {
      resource_key: resource_key,
      url: url,
      method: method
    }
  end

  def form_by_type
    case @form_type
    when 'login'
      [ 'user', '/users/sign_in', :post ]
    when 'edit_comment'
      comment_path = "/comments/#{@context['c']['id']}"
      ['edit_comment', comment_path, :put]
    end
  end

  def form_method
    return :post if form_special_methods?
    @form[:method]
  end

  def form_special_methods?
    @form[:method] == :patch || @form[:method] == :put || @form[:method] == :delete
  end
end

Liquid::Template.register_tag('form', FormTag)
<!-- app/views/tags/form_tag/form.html.erb -->
<form accept-charset="UTF-8"
      id="<%= @form_html_options[:id] %>"
      class="<%= @form_html_options[:class] %>"
      method="<%= form_method %>"
      action="<%= @form[:url] %>"
      data-remote="<%= @form_html_options[:remote] || false %>">
  <%= hidden_field_tag :authenticity_token, @form_token %>

  <% if form_special_methods? %>
    <%= hidden_field_tag("_method", @form[:method]) %>
  <% end %>

  <%= @html_block.html_safe %>
</form>

I can do this work putting the instance variables initialization inside the render function, but I want to know if is right do this and if it have some bad implications. Or if this code have something wrong that are causing this behaviour.

Thanks...

karlmdavis commented 8 years ago

I'll throw in a "me too" on this: running into the same problem with a custom tag I've got.

fraank commented 8 years ago

+1

ErvalhouS commented 6 years ago

+1

Convincible commented 6 years ago

+1

FYI: https://github.com/jekyll/jekyll/issues/176#issuecomment-6763686

Convincible commented 6 years ago

Solution:

Variables that must change for each forloop should not be @instance variables. Instead, set these in the render method (or thereafter) as temporary variables (e.g. var not @var).

Explanation:

As far as I can tell, initialize is called only once at the start of a forloop, and the whole class remains alive for the duration of that forloop. Thus, any instance variable carries its values from loop to loop.