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

Sitemap multilanguage problem #3215

Closed
sintro opened this issue Jul 13, 2016 · 3 comments
Closed

Sitemap multilanguage problem #3215

sintro opened this issue Jul 13, 2016 · 3 comments

Comments

@sintro
Copy link

sintro commented Jul 13, 2016

There are problem with sitemap generation when multiple locales are enabled.
The problem is in two places:
1.
https://github.com/refinery/refinerycms/blob/master/core/app/views/refinery/sitemap/index.xml.builder#L11
When this page_url (prepared as Hash) goes to refinery.url_for(page_url), it has locale key with locale, set in my browser (which is always will be :ru), and when the RouteFilter tries to generate url for current ::I18n.locale, it uses exactly params[:locale](look here https://github.com/refinery/refinerycms-i18n/blob/master/lib/refinery/i18n-filter.rb#L23). Probably, when sitemap.xml is called by search bots, there will be no :locale key in params, but I am not sure. The possible solution can be using modified line:

page.url.merge({:only_path => false, locale: locale})

This way, url_for will get modified params[:locale], as it is currently set buy @locales.each do |locale| loop. And the line::I18n.locale = locale can be removed.
2.
More serious problem takes place when the url_for gets String as page_url, which happens when the page with redirection is precessing (or even root page of site). In this case, the module RoutingFilter::RefineryLocales is not fired at all, so for all the locales in @locales array there will be absolutely identical urls. I don`t know how to solve this problem best, but there is at least stupid way to duplicate some code from here https://github.com/refinery/refinerycms-i18n/blob/master/lib/refinery/i18n-filter.rb#L30 to here https://github.com/refinery/refinerycms/blob/master/core/app/views/refinery/sitemap/index.xml.builder#L14, and this is obviously ugly solution. Any suggestions?

@sintro
Copy link
Author

sintro commented Jul 13, 2016

I just solved the problem with less duplications, but I think it is still not the best solution. All changes I did in sitemap\index.xml.builder, here is how it looks now:

xml.instruct!

xml.urlset "xmlns" => "http://www.sitemaps.org/schemas/sitemap/0.9" do

  @locales.each do |locale|
    ::I18n.locale = locale
    ::Refinery::Page.live.in_menu.includes(:parts).each do |page|
     # exclude sites that are external to our own domain.
     page_url = if page.url.is_a?(Hash)
       # This is how most pages work without being overriden by link_url
       page.url.merge({:only_path => false, locale: locale})
     elsif page.url.to_s !~ /^http/
       # handle relative link_url addresses.
       raw_url = [request.protocol, request.host_with_port, page.url].join
       if defined?(RoutingFilter::RefineryLocales)
         filter = RoutingFilter::RefineryLocales.new
         filter.around_generate({}) do
           raw_url
         end
       end
     end

     # Add XML entry only if there is a valid page_url found above.
     xml.url do
       xml.loc refinery.url_for(page_url)
       xml.lastmod page.updated_at.to_date
     end if page_url.present? and page.show_in_menu?
    end
  end

end

@bricesanchez
Copy link
Member

Could you provide us a PR ? we will be able to see the changes and try to improve it.

@sintro
Copy link
Author

sintro commented Jul 14, 2016

Sure, will do it today

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants