-
Notifications
You must be signed in to change notification settings - Fork 87
make compatible with rails 7.1 (most likely will break for previous v… #87
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
I confirm this fixes the issue. @svenfuchs can you please merge and release this as a new major version? I think it's fine to drop support for pre-Rails 7.1 as users can just use older versions of this gem which is very stable. |
|
@simi can you handle this? |
|
Can also confirm this helps. if Gem.loaded_specs['routing-filter'].version > Gem::Version.new('0.7')
raise 'Check if PR https://github.com/svenfuchs/routing-filter/pull/87 has been merged and released. If yes, delete this monkey patch.'
end
# We cannot prepend a custom extension module here because we call `super` in this method which should call the Rails
# #find_routes-method and not the routing_filter's #find_routes-method which is broken.
# Instead, we override the whole module definition to fix it.
module ActionDispatchJourneyRouterWithFiltering
def find_routes(env)
path = env.is_a?(Hash) ? env['PATH_INFO'] : env.path_info
filter_parameters = {}
original_path = path.dup
@routes.filters.run(:around_recognize, path, env) do
filter_parameters
end
super(env) do |match, parameters, route|
parameters = parameters.merge(filter_parameters)
if env.is_a?(Hash)
env['PATH_INFO'] = original_path
else
env.path_info = original_path
end
yield [match, parameters, route]
end
end
endThis opens the module and overwrites the libary's current implementation within your application. Then add an initializer Dir.glob(Rails.root.join('lib/ext/**/*.rb')).sort.each do |filename|
require filename
endSee also: https://makandracards.com/makandra/40730-how-to-organize-monkey-patches-in-ruby-on-rails-projects You cannot use |
|
Thanks for this fix @nduitz ! It works for me on Rails 7.1.4 - I really hope somebody is able to merge and release this and that the gem has not just become abandonware. |
|
@simi @svenfuchs please merge this and release as a new major version? |
|
In case anyone is unable to change their project structure as @begerdom suggests in #87 (comment), you may use the below snippet to override. Just put it in a file under # Removing the (existing) broken find_routes override from the routing-filter gem first
ActionDispatchJourneyRouterWithFiltering.remove_method(:find_routes)
module CustomOverridesActionDispatchJourneyRouterWithFiltering
def find_routes(env)
path = env.is_a?(Hash) ? env['PATH_INFO'] : env.path_info
filter_parameters = {}
original_path = path.dup
@routes.filters.run(:around_recognize, path, env) do
filter_parameters
end
##### OVERRIDE STARTS #####
super(env) do |match, parameters, route|
parameters = parameters.merge(filter_parameters)
if env.is_a?(Hash)
env['PATH_INFO'] = original_path
else
env.path_info = original_path
end
yield [match, parameters, route]
end
##### OVERRIDE ENDS #####
end
end
ActionDispatch::Journey::Router.send(:prepend, CustomOverridesActionDispatchJourneyRouterWithFiltering) |
|
I think it is good time to sunset this gem actually. I can try to release one of the last versions, but if I understand it properly, same can be achieved with vanilla Rails simply today. Maybe worth to write some migration guide. Is there any public project I can check and migrate and link to it in README? |
This gem was only utilized in the test sandbox, but users are expected to configure locale routing themselves. Since this gem does not support rails 7. svenfuchs/routing-filter#87
This gem was only utilized in the test sandbox, but users are expected to configure locale routing themselves. Since this gem does not support rails 7. svenfuchs/routing-filter#87
This gem was only utilized in the test sandbox, but users are expected to configure locale routing themselves. Since this gem does not support rails 7. svenfuchs/routing-filter#87
This gem was only utilized in the test sandbox, but users are expected to configure locale routing themselves. Since this gem does not support rails 7. svenfuchs/routing-filter#87
This gem was only utilized in the test sandbox, but users are expected to configure locale routing themselves. Since this gem does not support rails 7. svenfuchs/routing-filter#87
This gem was only utilized in the test sandbox, but users are expected to configure locale routing themselves. Since this gem does not support rails 7. svenfuchs/routing-filter#87
update version to 5.0.2 override the routing-filter/find_routes as recommended in svenfuchs/routing-filter#87 move complex conditions to methods in i18n-filter clarify logic in find_or_set_local remove flag icons as they are no longer used
|
@simi I'm happy to help with maintenance of this gem if you'd like. I don't think it should be sunset; yes you can achieve the same thing with in your routes.rb with complex nested scopes, but this gem simplifies it a great deal. Please let me know if I can help. |
|
@johnnyshields ok, let's keep it maintained. I was living under assumption, it is actually simpler using new router than using this gem. Which features are you using @johnnyshields? |
Actually, since this issue was opened, I’ve been able to cover everything I need using Rails routing defaults.
What parts do you think are missing from this section of the Rails I18n guide? If anything important is missing, I agree, we should definitely extend the Rails guides to cover it. As for now, I am using this: Rails.application.routes.draw do
post "/locale", to: "locales#update"
scope "(:locale)", locale: /#{I18n.available_locales.join("|")}/ do
get "/help", to: "pages#help"
resources :listings
...
# Defines the root path route ("/")
root "listings#index"
end
endFor me, it works pretty good. Sure, it does not have filters (just put the routs outside locale scope) and conditionally prepending the locale. I never used expanding feature. |
|
@viktorianer I like this approach as well for recognition. But how do you solve URL generation? That's another feature this gem gave you by actually hooking into the routing engine. Every helper that relies on |
To be honest, I hadn't personally encountered issues with URL generation in my setup, so I wasn't aware this was a specific problem the The standard Rails way to achieve automatic locale inclusion in generated URLs without a gem like I use a controller concern, which partially use the following ...
extend ActiveSupport::Concern
included do
around_action :switch_locale
end
# Setting the Locale from URL Params, e.g. URLs like www.example.com/en/books
def default_url_options
{ locale: I18n.locale }
end
...You can see this in action in my project https://staging.chargeincluded.eu/. I hope I understood your point :). |
|
@viktorianer Ah, thanks. I overlooked the section in the Rails guide that solves the specific URL transformation from |
|
Hmmm I'll give the vanilla Rails way a try in our app and report back. Need 1-2 weeks as I'm travelling. |
This seems to work fine in our project for filters to work again with rails >= 7.1.
Maybe I'll have the time to properly implement this with backwards compatibility.
For now this might help others :)
Should address this issue
#83