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

Configure headers in response setup #153

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

mabar
Copy link
Contributor

@mabar mabar commented Mar 17, 2019

  • BC break: no
  • doc PR: not needed

Moved headers configuration from initialize() method of DIC to Response service setup.
Useful for applications with multiple http layer implementations (we use psr-7 in Apitte). It should prevent mixing headers from nette/http if not used for current request

@mabar
Copy link
Contributor Author

mabar commented Mar 18, 2019

I am not sure how to fix tests. I tested it in my app and it should work properly.

@mabar mabar force-pushed the patch-1 branch 2 times, most recently from c23f7b6 to b834b0d Compare March 25, 2019 18:29
@dg dg force-pushed the master branch 2 times, most recently from 8fd7f28 to 51a304f Compare April 1, 2019 22:12
@mabar mabar force-pushed the patch-1 branch 2 times, most recently from 1718bd2 to 9b138d0 Compare July 6, 2019 14:32
@mabar
Copy link
Contributor Author

mabar commented Jul 6, 2019

@dg Ready

@dg
Copy link
Member

dg commented Jul 7, 2019

I'm afraid this is a BC break, which is not suitable for the patch version.

@mabar
Copy link
Contributor Author

mabar commented Jul 7, 2019

I thinked about it too. It should be BC break only in case that someone depends on fact that headers are sent and Response is not instantiated by user's app.

nette\application users are ok, only users of standalone nette/http should be affected and just in case they send response incorrectly without Response class.

As far as I know, Apitte is the only http-related nette extension, which is (almost) completely separated from nette/http and it will be not affected by this change until next version.

Do you know an other case which could be problematic? This PR is non-blocking for me, so it could wait. Just an architectural problem.

@mabar
Copy link
Contributor Author

mabar commented Jul 7, 2019

Maybe a compatibility-mode, which would be enabled by default? It would just initialize service in initialize() method, if enabled.

Something like this

@dg
Copy link
Member

dg commented Jul 7, 2019

In case it is non-blocking for you, I will postpone it to 3.1.

@mabar
Copy link
Contributor Author

mabar commented Jul 7, 2019

ok, thanks

@dg dg added this to the v3.1 milestone Jul 7, 2019
@dg dg force-pushed the master branch 8 times, most recently from f843ac1 to a472b8d Compare October 22, 2019 17:03
@dg dg force-pushed the master branch 2 times, most recently from 8b44821 to 052190c Compare October 31, 2019 15:11
@dg dg force-pushed the master branch 2 times, most recently from b1f6bb5 to 518f8e5 Compare December 11, 2019 19:47
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

2 participants