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

Fix rails/info routes for apps with globbing route #25430

Merged
merged 1 commit into from Jun 20, 2016

Conversation

Projects
None yet
5 participants
@nfm
Contributor

nfm commented Jun 18, 2016

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.

@rails-bot

This comment has been minimized.

rails-bot commented Jun 18, 2016

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.

@maclover7 maclover7 added the railties label Jun 18, 2016

@schneems

This comment has been minimized.

Member

schneems commented Jun 18, 2016

Idea is solid can you take a look at the failing tests

@nfm nfm force-pushed the nfm:fix-rails-info-routes-for-apps-with-globbing-route branch Jun 19, 2016

@nfm

This comment has been minimized.

Contributor

nfm commented Jun 19, 2016

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.

@prathamesh-sonpatki

View changes

railties/CHANGELOG.md Outdated
@@ -1,3 +1,7 @@
* Ensure /rails/info routes match in development for apps with a catch-all globbing route

This comment has been minimized.

@prathamesh-sonpatki

prathamesh-sonpatki Jun 19, 2016

Member

Please add a full stop at the end.

This comment has been minimized.

@prathamesh-sonpatki

prathamesh-sonpatki Jun 19, 2016

Member

Add /rails/info in backticks.

@nfm nfm force-pushed the nfm:fix-rails-info-routes-for-apps-with-globbing-route branch 2 times, most recently Jun 19, 2016

@nfm

This comment has been minimized.

Contributor

nfm commented Jun 19, 2016

Thanks @prathamesh-sonpatki - updated!

get "rails/info/properties"
assert_equal 200, last_response.status
end

This comment has been minimized.

@prathamesh-sonpatki

prathamesh-sonpatki Jun 19, 2016

Member

Should we add a test for rails/info too? That's the only one missing here.

@nfm nfm force-pushed the nfm:fix-rails-info-routes-for-apps-with-globbing-route branch Jun 19, 2016

@nfm

This comment has been minimized.

Contributor

nfm commented Jun 19, 2016

@prathamesh-sonpatki - sure thing, updated!

Fix rails/info routes for apps with globbing route
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 nfm force-pushed the nfm:fix-rails-info-routes-for-apps-with-globbing-route branch to 9d513e0 Jun 20, 2016

@nfm

This comment has been minimized.

Contributor

nfm commented Jun 20, 2016

@schneems just rebased onto master now that #25436 is merged, so the build should now pass unless we hit the weird actioncable test suite issue.

@schneems

This comment has been minimized.

Member

schneems commented Jun 20, 2016

Great, thanks!

@schneems schneems merged commit ce45972 into rails:master Jun 20, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

nfm added a commit to nfm/rails that referenced this pull request Jun 24, 2016

claudiob added a commit that referenced this pull request Mar 21, 2017

Remove 5.0.0 entry from 5.1.0 CHANGELOG [ci skip]
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

claudiob added a commit to claudiob/rails that referenced this pull request Mar 21, 2017

Remove duplicate entries for rails#25430 [ci skip]
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.

rafaelfranca added a commit that referenced this pull request Mar 21, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment