Migrating from rails 4 breaks the paths

[thebravoman]

I have an app using rails 4.2 and refinery working perfectly well. I've managed to upgrade everything to rails 5.1 except refinery.

After I add

gem 'refinerycms', '~> 4.0.0'
gem 'refinerycms-wymeditor'

to the Gemfile with nothing else

the following error occurs.

No route matches {:action=>"show", :controller=>"courses", :locale=>#<Course id: 5, created_at: "2017-08-01 17:12:55", updated_at: "2018-03-05 07:54:18", published_at: nil, picture_file_name: nil, picture_content_type: nil, picture_file_size: nil, picture_updated_at: nil, content_picture_id: nil, public_index_content: true, usage_type: "undefined", title: "Ddddldld">}, missing required keys: [:id]

The code is

<% path = main_app.polymorphic_url(object) %>

Which means that the main_app is now refinery I've not even add the refinery to the routes. Just changed the version in the Gemfile.

[bricesanchez]

Hi @thebravoman !

Refinery should be mounted to work properly:

Example: https://github.com/refinery/website/blob/master/config/routes.rb#L4

[thebravoman]

Initially I had it mounted with

mount Refinery::Core::Engine, at: Refinery::Core.mounted_path

but it does not work. So I thought of removing it from config/routes.rb and it still does not work.

That's what I mean that even after just adding to the Gemfile the error occurs.

[thebravoman]

Setting

config.railties_order = [:main_app, :all, Refinery::Core::Engine]

does not help

[thebravoman]

This here is the reason:

[16, 25] in /home/kireto/.rvm/gems/ruby-2.4.1/gems/refinerycms-i18n-4.0.1/lib/refinery/i18n/engine.rb
   16:       end
   17: 
   18:       config.to_prepare do
   19:         ::ApplicationController.module_eval do
   20:           def default_url_options
=> 21:             locale_param=(::Refinery::I18n.config.enabled? && ::I18n.locale != ::Refinery::I18n.default_frontend_locale) ? { :locale => ::I18n.locale } : {}
   22:             super.reverse_merge locale_param
   23:           end

Why are you overriding the default_url_options?

I already have my own implementation of default_url_options.

[thebravoman]

I have this configuration for i18n

Refinery::I18n.configure do |config|
  config.default_locale = :en
  config.default_frontend_locale = :en
end

but the call

::Refinery::I18n.config.enabled?

return false.

[thebravoman]

My understanding on default_url_options is to have the following implementation:

	def default_url_options options ={}
		if I18n.locale != I18n.default_locale
			options.merge!({locale: I18n.locale })
		else
			options.merge!({locale: nil })
		end
	end

In this way when there is a locale it is set in the options for default_url_options. If there is no, then a locale of null is set. This make polymorphic_path works, because it will have a locale of nil. Otherwise if it has no locale the object in

polymorphic_path(object) 

is understood as a locale and the error below occurs:

No route matches {:action=>"show", :controller=>"courses", :locale=>#<Course id: 5, created_at: "2017-08-01 17:12:55", updated_at: "2018-03-05 07:54:18", published_at: nil, picture_file_name: nil, picture_content_type: nil, picture_file_size: nil, picture_updated_at: nil, content_picture_id: nil, public_index_content: true, usage_type: "undefined", title: "Ddddldld">}, missing required keys: [:id]

Refinerycms should probably override default_url_options, but only if there is no currently implementation. Not doing it by default? What do you thing? @bricesanchez

[bricesanchez]

 ::Refinery::I18n.config.enabled? return false.

Refinery::I18n.config.enabled? has been removed in this commit:

refinery/refinerycms-i18n@89dbd52

and it appear 1 month later in this commit :

refinery/refinerycms-i18n@f61f58a#diff-78f6fc19856dc84d95f49a323a7a0735

It looks like it silently fail.

Could you provide a bugfix?

[thebravoman]

Here is an idea - do not change the default_url_options in the application controller.

Something like the code below. Refinery should not make all the other engines and the main app stick to its implementation of default_url_options and this should somehow be controlled. Perhaps with an if?

 config.to_prepare do
        if ::Refinery::I18n.config.enabled? 
          ::ApplicationController.module_eval do
            def default_url_options
              locale_param=(::Refinery::I18n.config.enabled? && ::I18n.locale != ::Refinery::I18n.default_frontend_locale) ? { :locale => ::I18n.locale } : {}
              super.reverse_merge locale_param
            end

            def find_or_set_locale
              ::I18n.locale = ::Refinery::I18n.current_frontend_locale

              if ::Refinery::I18n.has_locale?(locale = params[:locale].try(:to_sym))
                ::I18n.locale = locale
              elsif locale.present? && locale != ::Refinery::I18n.default_frontend_locale
                params[:locale] = ::I18n.locale.to_s = ::Refinery::I18n.default_frontend_locale.to_s
                redirect_to(params, :notice => "The locale '#{locale}' is not supported.") and return
              else
                ::I18n.locale = ::Refinery::I18n.default_frontend_locale
              end
              Globalize.locale = ::I18n.locale
            end

            prepend_before_action :find_or_set_locale
            protected :default_url_options, :find_or_set_locale
          end
        end

[thebravoman]

@bricesanchez I would try for a bug fix, but I do not fully understand the bug. I do not understand why is refinerycms redefining a method that has a perfectly valid implementation for the container app and for the other engines.

I am also not sure why is there a separate set of values ::Refinery::I18n.* that seems to do more or less the same as ::I18n.locale. Can't we just use the ::I18n.locale everywhere?

[thebravoman]

For example for my case the bug fix will look like this:

Initial:

def default_url_options
            locale_param=(::I18n.locale != ::Refinery::I18n.default_frontend_locale) ? { :locale => ::I18n.locale } : {}
            super.reverse_merge locale_param
          end

Fixed - with locale: nil

def default_url_options
            locale_param=(::I18n.locale != ::Refinery::I18n.default_frontend_locale) ? { :locale => ::I18n.locale } : {locale: nil}
            super.reverse_merge locale_param
          end

[bricesanchez]

Hi @thebravoman!

Could you test this PR to see if it fix your problem: refinery/refinerycms-i18n#74

Just add this in your Gemfile, then bundle:

gem 'refinerycms-i18n', git: 'https://github.com/refinery/refinerycms-i18n', branch: 'bugfix/default-url-options'

[thebravoman]

Hi @bricesanchez,
This solves only part of the problem.

In the routes I have

scope '/(:locale)', locale: /en|bg/ do 
	resources :courses
	# other resources following...
end

This means that I will have an url

/courses
/en/courses
/bg/courses

/courses and /en/courses are the same thing because the default locale is en.

So the user should be directed to /courses instead of /en/courses when he is browsing the site.
But if the default locale is always returned in default_url_options then the user will always be directed to /en/courses.

That's why we should return {locale: nil} when we are in the default locale.

But there is another problem with that. If the develop wants to have the locale as a param at the end of the ulr than we have these cases:

/courses
/courses?locale=en
/courses?locale=bg

If this is the case and we return locale:nil than the url for the default locale will be

/courses?locale=

In summary:
So the bug fix will always lead to /en/courses instead of /courses which is not the desired behavior.

The way I've implemented it is

	def default_url_options options ={}
		if I18n.locale != I18n.default_locale
			options.merge!({locale: I18n.locale })
		else
			options.merge!({locale: nil })
		end
	end

but this works only for the case where the locale is not a param but is part of the path.

[bricesanchez]

So the user should be directed to /courses instead of /en/courses when he is browsing the site.

This is what happens with my bugfix.

I think we will need a spec to confirm the behavior.

[bricesanchez]

Here is my proof for the bugfix : #3356