Check for Refinery::I18n's presence to fix #1533. #1534

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
@robyurkowski
Contributor

robyurkowski commented Mar 29, 2012

As discussed in #1533.

@phiggins

This comment has been minimized.

Show comment
Hide comment
@phiggins

phiggins Mar 29, 2012

Contributor

This is kind of messy, but I don't really see another way to do it other than having checks all over the place.

Perhaps refinery should always use I18n with a default locale of 'en' to avoid this kind of messiness. @parndt @ugisozols, WDYT?

Contributor

phiggins commented Mar 29, 2012

This is kind of messy, but I don't really see another way to do it other than having checks all over the place.

Perhaps refinery should always use I18n with a default locale of 'en' to avoid this kind of messiness. @parndt @ugisozols, WDYT?

@robyurkowski

This comment has been minimized.

Show comment
Hide comment
@robyurkowski

robyurkowski Mar 29, 2012

Contributor

Perhaps refinery should always use I18n with a default locale of 'en' to avoid this kind of messiness. @parndt @ugisozols, WDYT?

This, oh god, this. 👍

Contributor

robyurkowski commented Mar 29, 2012

Perhaps refinery should always use I18n with a default locale of 'en' to avoid this kind of messiness. @parndt @ugisozols, WDYT?

This, oh god, this. 👍

@parndt

This comment has been minimized.

Show comment
Hide comment
@parndt

parndt Mar 29, 2012

Member

You should never use defined?(Refinery::I18n) because you can just use Refinery.i18n_enabled?

Member

parndt commented Mar 29, 2012

You should never use defined?(Refinery::I18n) because you can just use Refinery.i18n_enabled?

@phiggins

This comment has been minimized.

Show comment
Hide comment
@phiggins

phiggins Mar 29, 2012

Contributor

@parndt ... but what if the constant isn't defined?

Contributor

phiggins commented Mar 29, 2012

@parndt ... but what if the constant isn't defined?

@ugisozols

This comment has been minimized.

Show comment
Hide comment
@ugisozols

ugisozols Mar 29, 2012

Member

@phiggins Refinery.i18n_enabled? checks if defined?(Refinery::I18n).

My proposal would be to simply change invalidate_cached_urls method to:

    def invalidate_cached_urls
        # other code ...
        ((Refinery.i18n_enabled? && Refinery::I18n.frontend_locales) || [::I18n.locale]).each do |locale|
          Rails.cache.delete(page.url_cache_key(locale))
          Rails.cache.delete(page.path_cache_key(locale))
        end
      end
    end

url_cache_key and path_cache_key, and cache_key shouldn't care about passed in locale.

Member

ugisozols commented Mar 29, 2012

@phiggins Refinery.i18n_enabled? checks if defined?(Refinery::I18n).

My proposal would be to simply change invalidate_cached_urls method to:

    def invalidate_cached_urls
        # other code ...
        ((Refinery.i18n_enabled? && Refinery::I18n.frontend_locales) || [::I18n.locale]).each do |locale|
          Rails.cache.delete(page.url_cache_key(locale))
          Rails.cache.delete(page.path_cache_key(locale))
        end
      end
    end

url_cache_key and path_cache_key, and cache_key shouldn't care about passed in locale.

@ugisozols

This comment has been minimized.

Show comment
Hide comment
@ugisozols

ugisozols Mar 29, 2012

Member

Btw I18n.locale is always returning something and by default it's en.

Member

ugisozols commented Mar 29, 2012

Btw I18n.locale is always returning something and by default it's en.

@parndt

This comment has been minimized.

Show comment
Hide comment
@parndt

parndt Mar 29, 2012

Member

@ugisozols better, but do we need a default Refinery::I18n module that gets extended by the presence of the other mixin perhaps? One that reports .frontend_locales to just be [::I18n.locale]

Member

parndt commented Mar 29, 2012

@ugisozols better, but do we need a default Refinery::I18n module that gets extended by the presence of the other mixin perhaps? One that reports .frontend_locales to just be [::I18n.locale]

@phiggins

This comment has been minimized.

Show comment
Hide comment
@phiggins

phiggins Mar 29, 2012

Contributor

Blech. Every class in Refinery shouldn't care whether or not I18n is enabled and what the various steps are to work around it.

Definitely put out a patch that will fix this particular issue, but the root problem still needs to be addressed.

Contributor

phiggins commented Mar 29, 2012

Blech. Every class in Refinery shouldn't care whether or not I18n is enabled and what the various steps are to work around it.

Definitely put out a patch that will fix this particular issue, but the root problem still needs to be addressed.

@parndt

This comment has been minimized.

Show comment
Hide comment
@parndt

parndt Mar 29, 2012

Member

@phiggins I agree, complete overhaul is the only option here. It's currently disgusting to deal with.

One option is just to move the I18n logic to core, but I'm not sure I love that option.

Member

parndt commented Mar 29, 2012

@phiggins I agree, complete overhaul is the only option here. It's currently disgusting to deal with.

One option is just to move the I18n logic to core, but I'm not sure I love that option.

@robyurkowski

This comment has been minimized.

Show comment
Hide comment
@robyurkowski

robyurkowski Mar 29, 2012

Contributor

At this point in time, IMO, we should not allow the user to remove I18n. It's not a huge dependency, and it'll substantially simplify our codebase + support just to be able to assume it exists.

I'm sure there is one, but I can't think of a good reason why we wouldn't make it part of the package.

Contributor

robyurkowski commented Mar 29, 2012

At this point in time, IMO, we should not allow the user to remove I18n. It's not a huge dependency, and it'll substantially simplify our codebase + support just to be able to assume it exists.

I'm sure there is one, but I can't think of a good reason why we wouldn't make it part of the package.

@phiggins

This comment has been minimized.

Show comment
Hide comment
@phiggins

phiggins Mar 29, 2012

Contributor

If the reason is just not wanting to add more things to core, I understand, however I disagree. There are tons of references to Refinery::I18n in core, and I think it's silly to keep it separate.

Also, I think Refinery's ease of translation is one of its killer features and should be celebrated rather than kept at arm's length.

Contributor

phiggins commented Mar 29, 2012

If the reason is just not wanting to add more things to core, I understand, however I disagree. There are tons of references to Refinery::I18n in core, and I think it's silly to keep it separate.

Also, I think Refinery's ease of translation is one of its killer features and should be celebrated rather than kept at arm's length.

@ugisozols

This comment has been minimized.

Show comment
Hide comment
@ugisozols

ugisozols Mar 29, 2012

Member

I like where this discussion is going. 👍 for moving I18n stuff to core.

Maybe we should open a new issue and keep the discussion there.

Member

ugisozols commented Mar 29, 2012

I like where this discussion is going. 👍 for moving I18n stuff to core.

Maybe we should open a new issue and keep the discussion there.

@phiggins

This comment has been minimized.

Show comment
Hide comment
@phiggins

phiggins Mar 29, 2012

Contributor

@ugisozols We could discuss it in the pull request with all the code you fixed to earn your $20. :trollface:

Contributor

phiggins commented Mar 29, 2012

@ugisozols We could discuss it in the pull request with all the code you fixed to earn your $20. :trollface:

@parndt

This comment has been minimized.

Show comment
Hide comment
@parndt

parndt Mar 30, 2012

Member

This was totally merged in 288ea25

Member

parndt commented Mar 30, 2012

This was totally merged in 288ea25

@parndt parndt closed this Mar 30, 2012

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