Add verify_host_path_consistency option #167

rogercampos · 2017-06-20T21:11:39Z

When using different hosts to identify different locales, it's
desirable to ensure that only coherent url's work on the application.

Mismatching between the locale assigned to a domain (ej: example.es) and
the language of the path (ej: '/cesta-de-la-compra') should be considered
404's.

This commit adds a configuration option to enable this behavior.

tagliala · 2017-06-20T22:31:19Z

lib/route_translator/extensions/route_set.rb

+          locale.to_s.gsub('native_', '')
+        end
+      end
+
      def add_localized_route(mapping, path_ast, name, anchor, scope, path, controller, default_action, to, via, formatted, options_constraints, options)


lib/route_translator/extensions/route_set.rb:26:7: C: Metrics/AbcSize: Assignment Branch Condition size for add_localized_route is too high. [26.89/23.15] def add_localized_route(mapping, path_ast, name, anchor, scope, path, controller, default_action, to, via, formatted, options_constraints, options)

This is going to add complexity to a method which is already very complex. Any chance to split this?

tagliala · 2017-06-20T22:32:31Z

lib/route_translator/extensions/route_set.rb

@@ -5,12 +5,42 @@
 module ActionDispatch
  module Routing
    class RouteSet
+      module VerificationLambdas


can we move this to another file, like lib/route_translator/translator/path.rb?

rogercampos · 2017-06-21T12:53:39Z

changes done!

coveralls · 2017-06-21T12:56:02Z

Coverage increased (+0.05%) to 99.338% when pulling bdf181b on rogercampos:feature/host-path-consistency-5 into 406bc9c on enriclluelles:develop.

tagliala · 2017-06-21T14:13:16Z

lib/route_translator.rb

@@ -5,6 +5,7 @@
 require File.expand_path('../route_translator/extensions', __FILE__)
 require File.expand_path('../route_translator/translator', __FILE__)
 require File.expand_path('../route_translator/host', __FILE__)
+require_relative 'route_translator/host_path_consistency_lambdas'


please prefer require over require_relative (same syntax as above)

I thought this was ok since this gem depends on rails 5, and rails 5 depends on ruby 2.2

tagliala · 2017-06-21T14:14:41Z

lib/route_translator/host_path_consistency_lambdas.rb

+  module HostPathConsistencyLambdas
+    module_function
+
+    def for_locale(locale)


Since for_locale is the only method used outside this module, you could move lambdas and sanitize_locale outside module_function.

Take a look at other files for example

tagliala · 2017-06-21T14:17:00Z

lib/route_translator/host_path_consistency_lambdas.rb

+    module_function
+
+    def for_locale(locale)
+      locale = sanitize_locale(locale)


Please avoid the function call and reassign locale.

Since .to_s.gsub('native_', '') is called three times.

I will then provide a global method to sanitize the locale (after the merge)

tagliala · 2017-06-21T14:17:46Z

lib/route_translator/extensions/route_set.rb

          translated_path_ast = ::ActionDispatch::Journey::Parser.parse(translated_path)
-          translated_mapping  = ::ActionDispatch::Routing::Mapper::Mapping.build(scope, self, translated_path_ast, controller, default_action, to, via, formatted, translated_options_constraints, anchor, translated_options)
+          translated_mapping  = __route_translator_translated_mapping(locale, self, translated_options, translated_path_ast, scope, controller, default_action, to, formatted, via, translated_options_constraints, anchor)


Please reneme this method to translate_mapping

The rationale was to minimize conflicts, since we're reopening a rails class.

rogercampos · 2017-06-27T14:36:56Z

proposed changes applied

coveralls · 2017-06-27T14:44:12Z

Coverage increased (+0.05%) to 99.342% when pulling ea041c7 on rogercampos:feature/host-path-consistency-5 into 406bc9c on enriclluelles:develop.

tagliala · 2017-06-27T14:51:36Z

@rogercampos perfect, thanks!

I just need an entry in the Readme to document this feature: https://github.com/enriclluelles/route_translator#available-configurations

Also, if this is not going to break things, I think this option should be set to true by default. What do you think?

rogercampos · 2017-06-27T15:03:23Z

Will add a note in the readme, I'll let you know.

I think this option should be true by default, mismatched routes should not work.

However, this is strictly speaking a breaking change in behavior. People using this gem and upgrading the version will have their apps behaving differently than before. Even if the previous behavior is considered "buggy", following semver the version upgrade should be major. I'd do an opt-in version and a minor release, with a default false, and changing later the default to true when doing a major version release.

tagliala · 2017-06-27T15:06:07Z

However, this is strictly speaking a breaking change in behavior. People using this gem and upgrading the version will have their apps behaving differently than before. Even if the previous behavior is considered "buggy", following semver the version upgrade should be major. I'd do an opt-in version and a minor release, with a default false, and changing later the default to true when doing a major version release.

Got it. I will release a version 5.x with the bugfix and add a note to change the default to true in 6.0.0 👍

When using different hosts to identify different locales, it's desirable to ensure that only coherent url's work on the application. Mismatching between the locale assigned to a domain (ej: example.es) and the language of the path (ej: '/cesta-de-la-compra') should be considered 404's. This commit adds a configuration option to enable this behavior.

rogercampos · 2017-06-27T15:19:54Z

Readme updated!

If you consider the current behavior a bug, then when doing a major release you can just remove the configuration option and make it a default behavior.

coveralls · 2017-06-27T15:23:31Z

Coverage increased (+0.05%) to 99.342% when pulling 102fdbc on rogercampos:feature/host-path-consistency-5 into 406bc9c on enriclluelles:develop.

* release/5.5.0: Update changelog and version Update changelog and version Refactor `require` in test folder Refactor `require` in /lib folder Move locale sanitizer to its own module Add verify_host_path_consistency option (#167)

tagliala · 2017-07-17T05:22:05Z

Hi @rogercampos,

Integrating lambdas in host locales was quite easy (https://github.com/enriclluelles/route_translator/tree/feature/integrate-lambdas-into-host)

I want to take a step further, so I'm considering to drop "native locale" routes: https://github.com/enriclluelles/route_translator/tree/drop-native-routes

Let's say you have %i[es fr] available locales, with es as default locale.

Route translator configuration:

RouteTranslator.config do |config|
  config.host_locales = {
    'es.lvh.me' => :es,
    'fr.lvh.me' => :fr,
  }
end

Routes:

Rails.application.routes.draw do
  localized do
    resources :cars, only: :index
  end
end

es.yml

es:
  routes:
    cars: 'coches'

fr.yml

fr:
  routes:
    cars: 'voitures'

In Route Translator 5 with lambdas, you can use:

es.lvh.me, with es routes;
fr.lvh.me, with fr routes;
whatever.lvh.me, with es routes;

Problems are:

whatever hosts only work with the default locale;
non default locale domain have duplicate routes: fr.lvh.me/voitures and fr.lvh.me/fr/voitures will both work.

Dropping native routes will solve the second issue but not the first one

What I would like to accomplish:

host locales only respond to native_routes of their locale
non-host locales will only respond to routes of all locales

Do you have any idea on how achieving this?

rogercampos · 2017-07-17T13:13:48Z

Hi @tagliala !

Sorry I don't know all the possible features supported by route_translator and I think I'm missing something. You said in your previous comment:

In Route Translator 5 with lambdas, you can use:

es.lvh.me, with es routes;

fr.lvh.me, with fr routes;

whatever.lvh.me, with es routes;

Why whatever.lvh.me would work at all? To me, if you're using host-based localization, you have to define a complete mapping of host -> locale for any host supported by your application.

tagliala · 2017-07-17T13:25:21Z

Why whatever.lvh.me would work at all?

That's the point. IMHO it should not work

Anyway, if someone is using this gem to support, let's say it.example.com, fr.example.com, and www.example.com (with both fr and it routes under /fr and /it), they cannot upgrade to the new major release

I'm not using recent versions of Route Translator in any production project. I could start a new issue, release a 6.0.0 alpha version and wait for feedbacks. What do you think?

rogercampos · 2017-07-17T13:41:58Z

You're right, that use case you mention would not work. But I cannot think of any legit reason to do that, really. Seo-wise for example is all problems: duplicated content?

The only scenario where that would be useful is if someone is doing a transition from one model to the other, and wants to gradually migrate domains. I.e.: you have your app under example.com/{es,it,fr} and you want to migrate to a host based model, but first you want to have example.fr + example.com/{es,it}, etc. We did that in our company years ago.

However I think that's a minor use case. I agree you could propose the change and see if there's much negative feedback. There would be only two ways to localize your app: either host-based or prefix-based, with no mix between them.

tagliala · 2017-07-23T12:05:58Z

@rogercampos thanks for your help.

I've released an alpha version of Route Translator 6.0.0 which integrates your work into the host locales option and cannot be disabled and I've opened a new thread, #171, for feedbacks, issues and discussion

rogercampos mentioned this pull request Jun 20, 2017

Issue with host-based locale and non-native routes #91

Closed

tagliala self-requested a review June 20, 2017 22:30

tagliala reviewed Jun 20, 2017

View reviewed changes

rogercampos force-pushed the feature/host-path-consistency-5 branch from 5f8de1d to bdf181b Compare June 21, 2017 12:49

tagliala reviewed Jun 21, 2017

View reviewed changes

rogercampos force-pushed the feature/host-path-consistency-5 branch from bdf181b to ea041c7 Compare June 27, 2017 14:36

rogercampos force-pushed the feature/host-path-consistency-5 branch from ea041c7 to 102fdbc Compare June 27, 2017 15:18

tagliala merged commit 6970384 into enriclluelles:develop Jun 28, 2017

tagliala added bug enhancement labels Jun 29, 2017

tagliala added this to the 5.5.0 milestone Jun 29, 2017

tagliala mentioned this pull request Jul 23, 2017

Avoid duplicate routes in host locales #171

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add verify_host_path_consistency option #167

Add verify_host_path_consistency option #167

rogercampos commented Jun 20, 2017

tagliala Jun 20, 2017

tagliala Jun 20, 2017

rogercampos commented Jun 21, 2017

coveralls commented Jun 21, 2017 •

edited

Loading

tagliala Jun 21, 2017 •

edited

Loading

rogercampos Jun 27, 2017

tagliala Jun 21, 2017 •

edited

Loading

tagliala Jun 21, 2017 •

edited

Loading

tagliala Jun 21, 2017 •

edited

Loading

rogercampos Jun 27, 2017

rogercampos commented Jun 27, 2017

coveralls commented Jun 27, 2017 •

edited

Loading

tagliala commented Jun 27, 2017

rogercampos commented Jun 27, 2017

tagliala commented Jun 27, 2017 •

edited

Loading

rogercampos commented Jun 27, 2017

coveralls commented Jun 27, 2017 •

edited

Loading

tagliala commented Jul 17, 2017 •

edited

Loading

rogercampos commented Jul 17, 2017

tagliala commented Jul 17, 2017

rogercampos commented Jul 17, 2017 •

edited

Loading

tagliala commented Jul 23, 2017

Add verify_host_path_consistency option #167

Add verify_host_path_consistency option #167

Conversation

rogercampos commented Jun 20, 2017

tagliala Jun 20, 2017

Choose a reason for hiding this comment

tagliala Jun 20, 2017

Choose a reason for hiding this comment

rogercampos commented Jun 21, 2017

coveralls commented Jun 21, 2017 • edited Loading

tagliala Jun 21, 2017 • edited Loading

Choose a reason for hiding this comment

rogercampos Jun 27, 2017

Choose a reason for hiding this comment

tagliala Jun 21, 2017 • edited Loading

Choose a reason for hiding this comment

tagliala Jun 21, 2017 • edited Loading

Choose a reason for hiding this comment

tagliala Jun 21, 2017 • edited Loading

Choose a reason for hiding this comment

rogercampos Jun 27, 2017

Choose a reason for hiding this comment

rogercampos commented Jun 27, 2017

coveralls commented Jun 27, 2017 • edited Loading

tagliala commented Jun 27, 2017

rogercampos commented Jun 27, 2017

tagliala commented Jun 27, 2017 • edited Loading

rogercampos commented Jun 27, 2017

coveralls commented Jun 27, 2017 • edited Loading

tagliala commented Jul 17, 2017 • edited Loading

rogercampos commented Jul 17, 2017

tagliala commented Jul 17, 2017

rogercampos commented Jul 17, 2017 • edited Loading

tagliala commented Jul 23, 2017

coveralls commented Jun 21, 2017 •

edited

Loading

tagliala Jun 21, 2017 •

edited

Loading

tagliala Jun 21, 2017 •

edited

Loading

tagliala Jun 21, 2017 •

edited

Loading

tagliala Jun 21, 2017 •

edited

Loading

coveralls commented Jun 27, 2017 •

edited

Loading

tagliala commented Jun 27, 2017 •

edited

Loading

coveralls commented Jun 27, 2017 •

edited

Loading

tagliala commented Jul 17, 2017 •

edited

Loading

rogercampos commented Jul 17, 2017 •

edited

Loading