samvera-labs / hydra_plugins_wg

Work area for the Hydra Plugins Working Group
Apache License 2.0
1 stars 0 forks source link

Adds *must* use routing proxy to guidelines #25

Closed afred closed 7 years ago

afred commented 7 years ago

Closes #23.

afred commented 7 years ago

@projecthydra-labs/hydra_plugins_wg review and merge?

afred commented 7 years ago

@jcoyne justification is mentioned in the issue comments.

Since the the implicit routing proxy changes depending on context, I think it's better if the plugin developer does not assume what that context is.

Perhaps it should be reworded to say:

When using URL helpers generated by Rails routes, plugins should use the explicit routing helper, and should not use the implicit routing helper.

Also, an example of a "good" vs "bad" code snippet here would probably be helpful.

jcoyne commented 7 years ago

This goes against idiomatic rails practices :-1:

afred commented 7 years ago

@jcoyne how is it going against idiomatic rails?

mbarnett commented 7 years ago

I'd love to see a citation on routing proxies being unidiomatic. It's a solution explicitly endorsed in the Rails Guides.

The major "idiomatic" Rails code bases use it quite a bit. Spree uses it extensively, to point at a well-known example.

jcoyne commented 7 years ago

You'll notice that none of the examples here use the explicit main_app route proxy: http://guides.rubyonrails.org/routing.html#path-and-url-helpers

Engines can be considered miniature applications that provide functionality to their host applications. A Rails application is actually just a "supercharged" engine, with the Rails::Application class inheriting a lot of its behavior from Rails::Engine.

So, if Rails::Application is a subclass of Rails::Engine, one should always prefix any route helper with main_app? That is not something I'm seeing.

afred commented 7 years ago

So...

For plugins that are Engines, I think it's more prudent to follow the Rails docs for Engines, yeah?

I can't think of a situation where it would be safer, or better code, to leave off the routing proxy and just assume that the implicit routing proxy is the one you want.

afred commented 7 years ago

@projecthydra-labs/hydra_plugins_wg can we get 👍 or 👎 on this from our group? So far, we have just one, from @mbarnett. Thanks!

afred commented 7 years ago

PR modifications needed:

cjcolvar commented 7 years ago

:+1: