-
-
Notifications
You must be signed in to change notification settings - Fork 842
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
Harden Headers #2721
Harden Headers #2721
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This definitely should have tests. We could add a header
subfolder to the integration test directory?
Or alternatively, construct a fake middleware pipe with a fake handler, and use unit tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stylistic comment. Also would be good to see some basic integration tests (ping /
, check for headers)
I'm thinking maybe a |
As a note I'm not sure why the test are failing, it seems to be that it's hitting rate limits or something? Everything is working locally 100% just fine and all test are passing on my machine. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All looks good to me from a headers point of view!
FIgured it out. The |
651ebbb
to
679fa91
Compare
Unfortunately it probably should, as tests break without it. There's a bunch of stuff that shouldn't though. From chat: I think it might be easiest here to:
|
679fa91
to
adb3fdd
Compare
I've re-based correctly this time, but it appears that test unrelated to this PR are failing again. I'm not entirely sure what's going on here. Maybe @askvortsov1 can take a look when he gets a chance? |
[ci skip] [skip ci]
[ci skip] [skip ci]
8008832
to
2f9819a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed up tests, moved those commits out to master to keep this PR clean. @davwheat did you self-request review because you had some concerns?
@tankerkiller125 No, sorry, just wanted to clear my review as I hadn't tested it locally after the rebase and stuff. No real changes have taken place, and Sasha has no issues with it, so it's good with me. |
**Partial Fix #353 **
Changes proposed in this pull request:
Reviewers should focus on:
Confirmed
composer test
).Additional PRs Required