locomotivecms / steam

The rendering stack used by both Wagon and Station (new name of the engine). It includes the rack stack and the liquid drops/filters/tags.
MIT License
38 stars 59 forks source link

YAMLLoaders::Page break with I18n pages #163

Open proxygear opened 5 years ago

proxygear commented 5 years ago

I try to push an I18n page to production from staging data, using the following command line :

wagon deploy production -d -v -e staging

And get the following errors :

Pushing Pages

# Error description:
no implicit conversion of Symbol into Integer

# Backtrace:

    /Users/my_user/.rvm/gems/ruby-2.5.3/gems/locomotivecms_steam-1.5.0/lib/locomotive/steam/adapters/filesystem/yaml_loaders/page.rb:105:in `[]='
    /Users/my_user/.rvm/gems/ruby-2.5.3/gems/locomotivecms_steam-1.5.0/lib/locomotive/steam/adapters/filesystem/yaml_loaders/page.rb:105:in `block in complete_attributes_from_data'
    /Users/my_user/.rvm/gems/ruby-2.5.3/gems/locomotivecms_steam-1.5.0/lib/locomotive/steam/adapters/filesystem/yaml_loaders/page.rb:94:in `each'
    /Users/my_user/.rvm/gems/ruby-2.5.3/gems/locomotivecms_steam-1.5.0/lib/locomotive/steam/adapters/filesystem/yaml_loaders/page.rb:94:in `complete_attributes_from_data'
    /Users/my_user/.rvm/gems/ruby-2.5.3/gems/locomotivecms_steam-1.5.0/lib/locomotive/steam/adapters/filesystem/yaml_loaders/page.rb:76:in `block in load_data'
    /Users/my_user/.rvm/gems/ruby-2.5.3/gems/locomotivecms_steam-1.5.0/lib/locomotive/steam/adapters/filesystem/yaml_loaders/page.rb:62:in `each'
    /Users/my_user/.rvm/gems/ruby-2.5.3/gems/locomotivecms_steam-1.5.0/lib/locomotive/steam/adapters/filesystem/yaml_loaders/page.rb:62:in `load_data'
    /Users/my_user/.rvm/gems/ruby-2.5.3/gems/locomotivecms_steam-1.5.0/lib/locomotive/steam/adapters/filesystem/yaml_loaders/page.rb:36:in `load'

Here is a sample of the pointed code : https://github.com/locomotivecms/steam/blob/master/lib/locomotive/steam/adapters/filesystem/yaml_loaders/page.rb

def complete_attributes_from_data(attributes, data, locale)
  return if attributes.nil? # shouldn't happen (undeleted page? local files out of date?)

  # this is super important to handle correctly url type settings in sections
  attributes[:_id] = data['id']

  # required by pages which are not present locally
  attributes[:_fullpath] = data['fullpath']

  # set the attributes
  %i(
    title slug redirect_url seo_title meta_description meta_keywords
    listed published position
    sections_content sections_dropzone_content editable_elements raw_template
  ).each do |name|
    next if (value = data[name.to_s]).nil?

    if name == :editable_elements
      update_editable_elements(attributes, value, locale)
    elsif %i(listed published position).include?(name)
      attributes[name] = value
    elsif name != :raw_template || (name == :raw_template && data['handle'].blank?)
      (attributes[name] ||= {})[locale] = value
    end
  end
end

The problem happen in the (attributes[name] ||= {})[locale] = value

Fact is attributes contains already data from the initial language, and so collide.

# attributes (coming from an :en page) looks like this
attributes = {title: 'Hello world'}

# and data (coming from a :fr page) looks like this
data = {title: 'Bonjour le monde'}

# so what happen is :
name = :title
locale = :en
(attributes[name] ||= {})[locale] = value
proxygear commented 5 years ago

I tried a dirty fix :

  puts "for #{name} #{locale}"
  if (attributes[name].is_a?(Hash))
    puts "add"
    attributes[name][locale] = value
  else
    puts "fixe"
    attributes[name] = {locale => attributes[name]}
  end
  #(attributes[name] ||= {})[locale] = value

However it does not work, as other data value can be already Hash, like sections_content and editable_elements. I guess somehow, attributes should start with an empty Hash.

I guess load_tree and load_data should extract their data separately. Then a 3rd method should handle the final merge.

EDIT : the problem was not there but upper in the process

proxygear commented 5 years ago

Looking at the code of lib/locomotive/steam/adapters/filesystem/yaml_loaders/page.rb, I think it would need some rework. My intuition would be to create a PageLeaf model that contains all the merging methods, leaving the page.rb responsible of the list ?

proxygear commented 5 years ago

The Travis CI failed. I corrected the specs except spec/integration/repositories/page_repository_spec.rb But I get the counters expecting different values depending of Filesystem or MongoDB cases. My intuition is that the spec/fixtures/mongodb files need to be updated to reflect the changes I did in the spec/fixtures/default/app/views/pages ... Can someone point me how to do so ?

I make a extra pull request to highlight where I'm stuck.