bridgetownrb / bridgetown

A next-generation progressive site generator & fullstack framework, powered by Ruby
https://www.bridgetownrb.com
MIT License
1.16k stars 112 forks source link

Select in "Bridgetown::Localizable#all_locales" is not strict enough #660

Closed svoop closed 1 year ago

svoop commented 1 year ago

With two locales :de and :en and given the following structure:

/_pages
  /profile
    /index.de.md
    /index.en.md
  /contact
    /index.de.md
    /index.en.md

Doing a resource.all_locales on any of the four resources will return an array of all four resources due to the select filtering by slug only:

https://github.com/bridgetownrb/bridgetown/blob/1aef49d3ab34686f6cbc942572aa2093d515d072/bridgetown-core/lib/bridgetown-core/concerns/localizable.rb#L15

(I'm not quite sure whether this is a bug or the intended behavior clashing with how I organize my pages. However, it also happens if you have say two product subdirs each with license.xx.md.)

Bridgetown Version: 1.1.0 (code unchanged in Main as of today)

To Reproduce

Current behavior The select filters by slug across all resources.

Expected behavior The select should filter by slug across all resources of the same subdir only.

Maybe add support for canonical URLs? Something along the lines of:

jaredcwhite commented 1 year ago

Thanks for catching that! Hmm… seems like we'd want to add an extra filter to ensure the files are not coming from different origin paths.

svoop commented 1 year ago

I'm working around it with helpers:

# Return the canonical (not localized) URL
def canonical(url)
  url.sub(%r{^/(?:#{I18n.available_locales.join('|')})/}, '/')
end

# Return the localized resources matching the given one
def localized_resources(resource)
  canonical_relative_url = canonical(resource.relative_url)
  resource.all_locales.select do |localized_resource|
    canonical_relative_url == canonical(localized_resource.relative_url)
  end
end

One ugly hack, but canonical URLs are really essential to do SEO right on i18nized sites. Even thought about monkey patching in resource.relative_canonical_url and resource.absolute_canonical_url for a second, but that's guaranteed to break sooner or later.

svoop commented 1 year ago

@jaredcwhite Since the all_locales is fixed (thanks, @vvveebs !) – what do you think about the other idea mentioned above to add two methods to resources:

In case you find this useful, I'd create a new issue for it. Cheers!

jaredcwhite commented 1 year ago

what do you think about the other idea mentioned above to add two methods to resources

@svoop Sure, if that's useful for SEO why not? If you could write up an issue that would be great. Thanks!