Skip to content
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

Allow public access to the dev/health url and change response to 500 #34

Merged
merged 2 commits into from Jul 12, 2016

Conversation

stojg
Copy link

@stojg stojg commented Jun 17, 2016

According to the documentation, the dev/health route should be publically accessible, this PR removes the current permission check.

In addition to this, the default return error code is 500 (server error) instead of 404 (which is regarded as a client error).

@helpfulrobot
Copy link

@stojg, thanks for your PR! By analyzing the blame information on this pull request, I identified @sminnee, @assertchris and @halkyon to be potential reviewers

@dhensby
Copy link
Contributor

dhensby commented Jun 18, 2016

It would be good for the old url to forward to the new one, otherwise this is a pretty significant change and would need to be in the next major release.

@stojg
Copy link
Author

stojg commented Jun 19, 2016

I would prefer to keep the old URL and allow open access to the dev/health endpoint since that is the documented behaviour but was changed with the changes in https://github.com/silverstripe-labs/silverstripe-environmentcheck/pull/17/files but @tractorcow want everything under dev/ to be protected.

@tractorcow
Copy link
Contributor

You need to add $usesDatabase = true to your tests now.

See silverstripe/silverstripe-framework#5557 (comment) for reference.

@tractorcow
Copy link
Contributor

Just merge #35 after tests pass, then rebase this. :D

@tractorcow tractorcow merged commit a8c0963 into master Jul 12, 2016
@tractorcow tractorcow deleted the public-access branch July 12, 2016 02:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants