Fix rails/info routes for apps with globbing route #25430
Conversation
|
Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @schneems (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
|
Idea is solid can you take a look at the failing tests |
nfm
force-pushed
the
fix-rails-info-routes-for-apps-with-globbing-route
branch
from
d06ec78
to
edb0035
Compare
|
Thanks @schneems. It looks like #25426 had the same railties test fail (https://travis-ci.org/rails/rails/jobs/138415607#L1368). #25436 looks like it's trying to fix this issue. The actioncable tests are failing intermittently for me locally. I don't think it's an ordering issue, I can re-run the actioncable suite with the same seed and have it pass most of the time, but still fail occasionally. I assume it's some kind of concurrency related issue. |
| @@ -1,3 +1,7 @@ | |||
| * Ensure /rails/info routes match in development for apps with a catch-all globbing route | |||
There was a problem hiding this comment.
Please add a full stop at the end.
There was a problem hiding this comment.
Add /rails/info in backticks.
nfm
force-pushed
the
fix-rails-info-routes-for-apps-with-globbing-route
branch
2 times, most recently
from
2a33a63
to
1a7c328
Compare
|
Thanks @prathamesh-sonpatki - updated! |
|
|
||
| get "rails/info/properties" | ||
| assert_equal 200, last_response.status | ||
| end |
There was a problem hiding this comment.
Should we add a test for rails/info too? That's the only one missing here.
nfm
force-pushed
the
fix-rails-info-routes-for-apps-with-globbing-route
branch
from
1a7c328
to
7148a03
Compare
|
@prathamesh-sonpatki - sure thing, updated! |
The /rails/info routes were inaccessible in apps with a catch-all globbing route, as they were being appended after the globbing route and would never be matched. See also ccc3ddb.
nfm
force-pushed
the
fix-rails-info-routes-for-apps-with-globbing-route
branch
from
7148a03
to
9d513e0
Compare
|
Great, thanks! |
Backport of rails#25430 to 5-0-stable
The entry for PR #25430 is currently present both in the CHANGELOG for Rails 5.0 and for Rails 5.1: https://github.com/rails/rails/blame/9d3a352777c2594123583b0bc02d0dd80f1e385b/railties/CHANGELOG.md#L61-L72 Since the PR was backported to 5-0-stable in #25499, I believe it should be removed from the CHANGELOG of 5.1, otherwise it looks like something changed from 5.0 to 5.1
The CHANGELOG for 5-0-stable includes the same change three times: https://github.com/rails/rails/blame/9d3a352777c2594123583b0bc02d0dd80f1e385b/railties/CHANGELOG.md#L61-L75 Indeed, the change was merged into 5.0 as part of e57b9e5 so it belongs to the "Rails 5.0.0" section.
Remove duplicate entries for #25430 [ci skip]
The /rails/info routes were inaccessible in apps with a catch-all globbing route, as they were being appended after the globbing route and would never be matched.
See also ccc3ddb.