Rename login_url method to login_url_with_optional_shop to avoid ambiguity with Rails' route helper method of the same name
#585
Conversation
swalkinshaw
requested review from
Hammadk,
EiNSTeiN-,
jamiemtdwyer and
chrisbutcher
There was a problem hiding this comment.
Makes sense. Unfortunately, a ton of apps use protected methods so this will be a breaking change for some folks.
Should this be a major version change?
Putting this here so it's top of mind, but I would love for us to move away from protected methods all together.
|
I've also hit this issue: Issue with I'd love to see this change. Maybe in v9. |
|
I've again spent several days tracking down this issue. Can you please merge it? That would be really appreciated! |
|
I'll rebase and ship this today. |
chrisbutcher
force-pushed
the
rename-login-url-method
branch
from
66a03d5
to
55a3ad0
Compare
chrisbutcher
force-pushed
the
rename-login-url-method
branch
from
e60cc43
to
7a1c6e8
Compare
|
cc: @Hammadk @EiNSTeiN- Can I get another quick review of this PR? |
|
👋 @Shopify/platform-dev-tools-education Can I also get another review from one of you? |
There was a problem hiding this comment.
looks good. @chrisbutcher lmk if you need me to 🎩, otherwise feel free to merge.
+1 to doing a major release for this. there may be some other breaking changes we can bundle in with a release this week
|
Thanks @nwtn !
Actually, that would be super helpful, if you don't mind :) |
|
Thanks guys for getting back on this right away! 👍 Edit: I guess the major version bump is to v10 now. |
chrisbutcher
changed the title
login_url method to login_url_with_optional_shop
chrisbutcher
changed the title
login_url method to login_url_with_optional_shoplogin_url method to login_url_with_optional_shop to avoid ambiguity with Rails' route helper method of the same name
|
@swalkinshaw would like to get another PR merged into |
|
I was really hoping this change got merged into the new major version bump... :( |
`login_url` potentially conflicts with Rails' route helper methods which are auto generated. Since this engine defines a `login` route, `login_url` would be the generated method. While this doesn't cause problems in normal use cases, if the same route is defined in the main app's route to point to a customized sessions controller (for example), the route helper method `login_url` will take precedence causing redirects to happen *without* the `shop` param. ie: ```ruby Rails.application.routes.draw do get 'login', to: 'sessions#new', as: :login mount ShopifyApp::Engine, at: '/' end ``` Even though this is guarding against an uncommon edge case, there's no reason to keep the conflicting method name. This simply renames it to `login_url_with_optional_shop`.
chrisbutcher
force-pushed
the
rename-login-url-method
branch
from
7a1c6e8
to
0bc124b
Compare
|
I just saw this got merged in v11. Wow! Thank you so much, for making a major version bump just for this PR! I really appreciate it! ❤️ 🎉 |
|
No worries! Sorry it slipped from our list of changes to include in v10 :) |
|
Could this be documented in the README as a breaking change when upgrading to v11? It's only mentioned in the changelog. |
login_urlpotentially conflicts with Rails' route helper methods which are auto generated. Since this engine defines aloginroute,login_urlwould be the generated method.While this doesn't cause problems in normal use cases, if the same route is defined in the main app's route to point to a customized sessions controller (for example), the route helper method
login_urlwill take precedence causing redirects to happen without theshopparam.ie:
Even though this is guarding against an uncommon edge case, there's no reason to keep the conflicting method name.
This simply renames it to
login_url_with_optional_shop.NOTE: I guess this is a potentially breaking change if anyone had overridden
login_urlpreviously even though it's a protected method.