Keep part when scope option has value #34656

albertoalmagro · 2018-12-08T22:12:06Z

Summary

When a route was defined within an optional scope, if that route didn't take parameters the scope was lost when using path helpers. This patch ensures scope is kept both when the route takes parameters or when it doesn't.

Fixes #33219

r? @gmcgibbon

albertoalmagro · 2018-12-08T22:56:51Z

r? @gmcgibbon

gmcgibbon · 2018-12-10T18:18:59Z

actionpack/lib/action_dispatch/journey/route.rb

@@ -51,13 +51,13 @@ def self.verb_matcher(verb)

      def self.build(name, app, path, constraints, required_defaults, defaults)
        request_method_match = verb_matcher(constraints.delete(:request_method))
-        new name, app, path, constraints, required_defaults, defaults, request_method_match, 0
+        new name, app, path, constraints, required_defaults, defaults, request_method_match, 0, {}


I think we can safely assume {} by default for scope_options . Also, I think we're far past the point of having too many positional arguments for this constructor (and Mapping's constructor). We should look at using keyword args instead, but that can be done in another PR. They both seem to be private API so we don't need deprecation cycles.

gmcgibbon · 2018-12-10T18:58:30Z

actionpack/lib/action_dispatch/journey/formatter.rb

@@ -67,7 +67,7 @@ def extract_parameterized_parts(route, options, recall, parameterize = nil)
          parameterized_parts = recall.merge(options)

          keys_to_keep = route.parts.reverse_each.drop_while { |part|
-            !options.key?(part) || (options[part] || recall[part]).nil?
+            (!options.key?(part) && !value_for_optional_scope?(part, route, recall)) || (options[part] || recall[part]).nil?


I find this hard to read. Maybe we can refactor, or space out the logic and add some comments?

I was able to make the condition a little bit more simple and removed also the auxiliary method value_for_optional_scope?.

albertoalmagro · 2018-12-10T21:28:55Z

@gmcgibbon Thanks for your review! Yes, I was also thinking about refactoring stuff here but wasn't sure if I should include it in another PR or within this one. My main concern was to have the commit that fixes the issue separated so it was clear what was fixing it. After having your opinion I think it makes sense to include some refactor improvements within this PR. I will submit them in separate commits and let you know when everything is ready for review again 👍

albertoalmagro · 2018-12-12T23:07:32Z

@gmcgibbon I've simplified a little bit the condition that fixes the bug by deleting the auxiliary method (it turned out that only first check was needed) and by simplifying boolean logic. I have squashed the changes for this fix but you can still compare both versions by checking the outdated comment above.

Regarding refactorings, I've included three commits, each of them makes a different improvement. They are separate so it is clearer for you and for the commit history what was done. As you said, they could go in different PRs but I decided to take the opportunity to add them here so the PR in global results on a better patch.

albertoalmagro · 2018-12-27T00:31:58Z

@gmcgibbon sorry I forgot to say it explicitly, this is ready for review. Could you please check it again? Thanks!

albertoalmagro · 2019-01-10T22:33:59Z

@gmcgibbon Sorry for pinging you again. Does this need anything else?

Thanks!

gmcgibbon

Sorry for the delay. Please add a changelog entry to the first commit. I would also opt to merge c40e6d9daa9a35e2f7f6dd500735f86eecb07be0 into the first commit too because its not a behaviour change, just syntax.

gmcgibbon · 2019-01-11T22:47:00Z

actionpack/lib/action_dispatch/routing/mapper.rb


-          defaults = (scope[:defaults] || {}).dup
-          scope_constraints = scope[:constraints] || {}
+          mapping_params = {


Nitpick: mapping_params could just b fed directly into the constructor here, same with route_params below.

albertoalmagro · 2019-01-12T11:20:23Z

@gmcgibbon thanks for your review! I've applied all your feedback:

CHANGELOG entry added to first commit.
Merged c40e6d9 into the first commit.
Fed mapping_params and route_params directly into the constructor.

gmcgibbon

Thanks for your patience. LGTM!

gmcgibbon · 2019-01-14T22:48:51Z

Actually, one last thing. It would seem I'm wrong about ActionDispatch::Routing::Mapper being private API. We have to revert the kwarg change on that class.

albertoalmagro · 2019-01-15T23:14:51Z

@gmcgibbon my commit message was wrong. It wasn't actually ActionDispatch::Routing::Mapper, but ActionDispatch::Routing::Mapper::Mapping. Which is private API.

Sorry for the confusion. I've changed my commit message, now everything should be good to go.

gmcgibbon · 2019-01-17T17:04:21Z

@albertoalmagro Thanks for clarifying. In that case, I agree this is good to go. Great work!

I can only merge doc PRs so a core member needs to look this over before merging. cc @rafaelfranca

albertoalmagro · 2019-01-19T10:20:10Z

Thanks! I've just rebased to fix a conflict on the CHANGELOG file.

albertoalmagro · 2019-02-02T16:13:59Z

r? @rafaelfranca

gmcgibbon · 2019-02-19T22:42:28Z

actionpack/CHANGELOG.md

@@ -7,7 +18,6 @@

    *Edouard Chin*

-


This is intentional, please revert this line.

My mistake! Didn't see that. Thanks @gmcgibbon for double checking ❤️

albertoalmagro · 2019-04-19T21:57:32Z

@rafaelfranca This is ready again, could you please review? Thanks

albertoalmagro · 2019-05-17T12:06:27Z

Fixed conflicts in CHANGELOG. Could anyone please take a look at this? 🙏

gmcgibbon · 2019-05-17T21:13:24Z

Sorry this is taking so long. I'm going to run this against an actual app and make sure we aren't introducing any regressions in terms of routing edge cases.

albertoalmagro · 2019-05-17T23:01:30Z

Thanks @gmcgibbon, I appreciate it 🙂

kamipo · 2019-05-18T09:44:34Z

actionpack/lib/action_dispatch/journey/route.rb

@@ -50,14 +50,15 @@ def self.verb_matcher(verb)
      end

      def self.build(name, app, path, constraints, required_defaults, defaults)
-        request_method_match = verb_matcher(constraints.delete(:request_method))
-        new name, app, path, constraints, required_defaults, defaults, request_method_match, 0, {}
+        new name: name, app: app, path: path, constraints: constraints,


Is there any worth changing new name, app, path, constraints, ... to new name: name, app: app, path: path, constraints: constraints, ...?
The self.build still have the dependency of remember parameters'
positions, and the new name: name, app: app, path: path, constraints: constraints, ... looks redundant to me.

Good point. We should update self.build to use keyword args too. We could even consider removing it, I don't see Route.build used anywhere other than tests unless it is used somewhere else magically. 🤔

Good catch @kamipo! As @gmcgibbon pointed out, Route.build wasn't necessary. My last commit removes it. Thanks for your reviews guys ❤️

When a route was defined within an optional scope, if that route didn't take parameters the scope was lost when using path helpers. This patch ensures scope is kept both when the route takes parameters or when it doesn't. Fixes #33219

This commit changes from constructor's argument list to keyword arguments in order to remove the dependency of remember parameters' positions. The constructor already provided a default value for `internal`, this commits takes the chance to also add default values for `precedence` and `scope_options`.

…nstructor This commit changes from constructor's argument list to keyword arguments in order to remove the dependency of remember parameters' positions. It also unifies all parameters extracted from the `scope` into `scope_params`, which also takes care of providing the default values for them.

@kamipo

After @kamipo CR feedback we realized `Route#build` wasn't used. As it is also private API being able to create Routes both with `#new` and `#build` was redundant.

gmcgibbon

👍

manueljacob · 2019-05-23T00:40:43Z

Thank you for implementing and merging this!

@rafaelfranca I didn’t find out how the releasing procedure works exactly. What’s the first possible release which could include this fix?

kamipo · 2019-05-23T04:14:48Z

actionpack/lib/action_dispatch/routing/mapper.rb


-          new set, ast, defaults, controller, default_action, scope[:module], to, formatted, scope_constraints, scope[:blocks] || [], via, options_constraints, anchor, options
+          new set: set, ast: ast, controller: controller, default_action: default_action,


Is there any worth changing new set, ast, defaults, controller, default_action, ... to new set: set, ast: ast, controller: controller, default_action: default_action, ...?
The self.build still have the dependency of remember parameters'
positions, and the new set: set, ast: ast, controller: controller, default_action: default_action, ... looks redundant to me.

👍 We should grep for usage and submit a followup PR to address this.

rafaelfranca · 2019-05-23T04:59:37Z

@manueljacob Good question. @gmcgibbon do you see any breaking change on this commit or it is just a bug fix? If it is a bug fix we can include in 6-0-stable

deivid-rodriguez · 2019-05-23T10:37:09Z

@albertoalmagro I'm curious whether this fixes #12178. Not that I care much these days, but it was the first issue I ever opened on Github 😃.

gmcgibbon · 2019-05-23T14:26:57Z

I believe fb9117e can be included in 6-0-stable. I don't see any breaking change besides the private API changes in other commits. Will backport.

We should look at making a test case for #12178 so we can validate it is fixed.

Backported in 85855d6.

rails/rails#34656 rails/rails@85855d6

PR rails/rails#34656 has recently been merged to rails master and backported to the rails 6-0-stable branch. It added an extra argument to the Mapper::Mapping.new method which makes it incompatible with the current version of route_translator This change is going to use `Mapper::Mapping.build` instead of `Mapper::Mapping.new` because the former preserved its signature across all Rails versions Fix: #195 Ref: rails/rails@85855d63fc

rails-bot bot added the actionpack label Dec 8, 2018

gmcgibbon reviewed Dec 10, 2018

View reviewed changes

gmcgibbon reviewed Jan 11, 2019

View reviewed changes

gmcgibbon approved these changes Jan 14, 2019

View reviewed changes

albertoalmagro mentioned this pull request Feb 19, 2019

Path helper picks up surrounding scope only if the URL has a parameter #33219

Closed

gmcgibbon reviewed Feb 19, 2019

View reviewed changes

gmcgibbon self-assigned this May 17, 2019

kamipo reviewed May 18, 2019

View reviewed changes

albertoalmagro added 4 commits May 22, 2019 23:03

Keep part when scope option has value

fb9117e

When a route was defined within an optional scope, if that route didn't take parameters the scope was lost when using path helpers. This patch ensures scope is kept both when the route takes parameters or when it doesn't. Fixes #33219

Remove Route#build as it wasn't used

20104ba

After @kamipo CR feedback we realized `Route#build` wasn't used. As it is also private API being able to create Routes both with `#new` and `#build` was redundant.

gmcgibbon approved these changes May 23, 2019

View reviewed changes

gmcgibbon merged commit eba859d into rails:master May 23, 2019

kamipo reviewed May 23, 2019

View reviewed changes

goduchesne added a commit to hooktstudios/route_translator that referenced this pull request Jul 10, 2019

Fix missing arguments for Mapper::Mapping.new for Rails 6

d0b9a07

rails/rails#34656 rails/rails@85855d6

louim mentioned this pull request Jul 10, 2019

Not working with Rails > 6.0.0.rc1 enriclluelles/route_translator#195

Closed

tagliala mentioned this pull request Jul 20, 2019

Add Rails > 6.0.0.rc1 compatibility enriclluelles/route_translator#197

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Keep part when scope option has value #34656

Keep part when scope option has value #34656

albertoalmagro commented Dec 8, 2018

albertoalmagro commented Dec 8, 2018

gmcgibbon Dec 10, 2018

gmcgibbon Dec 10, 2018

albertoalmagro Dec 12, 2018

albertoalmagro commented Dec 10, 2018

albertoalmagro commented Dec 12, 2018

albertoalmagro commented Dec 27, 2018

albertoalmagro commented Jan 10, 2019

gmcgibbon left a comment

gmcgibbon Jan 11, 2019

albertoalmagro commented Jan 12, 2019 •

edited

gmcgibbon left a comment

gmcgibbon commented Jan 14, 2019

albertoalmagro commented Jan 15, 2019

gmcgibbon commented Jan 17, 2019

albertoalmagro commented Jan 19, 2019

albertoalmagro commented Feb 2, 2019

gmcgibbon Feb 19, 2019

albertoalmagro Feb 20, 2019

albertoalmagro commented Apr 19, 2019

albertoalmagro commented May 17, 2019

gmcgibbon commented May 17, 2019

albertoalmagro commented May 17, 2019

kamipo May 18, 2019

gmcgibbon May 22, 2019

albertoalmagro May 22, 2019

gmcgibbon left a comment

manueljacob commented May 23, 2019

kamipo May 23, 2019

gmcgibbon May 23, 2019

rafaelfranca commented May 23, 2019

deivid-rodriguez commented May 23, 2019

gmcgibbon commented May 23, 2019 •

edited


		new set, ast, defaults, controller, default_action, scope[:module], to, formatted, scope_constraints, scope[:blocks] \|\| [], via, options_constraints, anchor, options
		new set: set, ast: ast, controller: controller, default_action: default_action,

Keep part when scope option has value #34656

Keep part when scope option has value #34656

Conversation

albertoalmagro commented Dec 8, 2018

Summary

albertoalmagro commented Dec 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albertoalmagro commented Dec 10, 2018

albertoalmagro commented Dec 12, 2018

albertoalmagro commented Dec 27, 2018

albertoalmagro commented Jan 10, 2019

gmcgibbon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albertoalmagro commented Jan 12, 2019 • edited

gmcgibbon left a comment

Choose a reason for hiding this comment

gmcgibbon commented Jan 14, 2019

albertoalmagro commented Jan 15, 2019

gmcgibbon commented Jan 17, 2019

albertoalmagro commented Jan 19, 2019

albertoalmagro commented Feb 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albertoalmagro commented Apr 19, 2019

albertoalmagro commented May 17, 2019

gmcgibbon commented May 17, 2019

albertoalmagro commented May 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmcgibbon left a comment

Choose a reason for hiding this comment

manueljacob commented May 23, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafaelfranca commented May 23, 2019

deivid-rodriguez commented May 23, 2019

gmcgibbon commented May 23, 2019 • edited

albertoalmagro commented Jan 12, 2019 •

edited

gmcgibbon commented May 23, 2019 •

edited