Skip to content

Configure headers in response setup - #153

Open
mabar wants to merge 1 commit into
nette:masterfrom
mabar:patch-1
Open

Configure headers in response setup#153
mabar wants to merge 1 commit into
nette:masterfrom
mabar:patch-1

Conversation

@mabar

@mabarmabar commented Mar 17, 2019

Copy link
Copy Markdown
Contributor
  • 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
Copy Markdown
ContributorAuthor

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

@mabar
mabarforce-pushed the patch-1 branch 2 times, most recently from c23f7b6 to b834b0dCompareMarch 25, 2019 18:29
@dg
dgforce-pushed the master branch 2 times, most recently from 8fd7f28 to 51a304fCompareApril 1, 2019 22:12
@mabar
mabarforce-pushed the patch-1 branch 2 times, most recently from 1718bd2 to 9b138d0CompareJuly 6, 2019 14:32
@mabar

mabar commented Jul 6, 2019

Copy link
Copy Markdown
ContributorAuthor

@dg Ready

@dg

dg commented Jul 7, 2019

Copy link
Copy Markdown
Member

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

@mabar

mabar commented Jul 7, 2019

Copy link
Copy Markdown
ContributorAuthor

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

mabar commented Jul 7, 2019

Copy link
Copy Markdown
ContributorAuthor

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

Something like this

@dg

dg commented Jul 7, 2019

Copy link
Copy Markdown
Member

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

@mabar

mabar commented Jul 7, 2019

Copy link
Copy Markdown
ContributorAuthor

ok, thanks

@dgdg added this to the v3.1 milestone Jul 7, 2019
@dg
dgforce-pushed the master branch 8 times, most recently from f843ac1 to a472b8dCompareOctober 22, 2019 17:03
@dg
dgforce-pushed the master branch 2 times, most recently from 8b44821 to 052190cCompareOctober 31, 2019 15:11
@dg
dgforce-pushed the master branch 2 times, most recently from b1f6bb5 to 518f8e5CompareDecember 11, 2019 19:47
@dg
dgforce-pushed the master branch 2 times, most recently from 7ae9bc0 to 0c8dfceCompareAugust 2, 2020 16:52
@dg
dgforce-pushed the master branch 4 times, most recently from ab1b02a to a2d51faCompareOctober 20, 2020 16:30
@dg
dgforce-pushed the master branch 6 times, most recently from 9ff6baa to 420d9a2CompareNovember 2, 2020 15:43
@dg
dgforce-pushed the master branch 6 times, most recently from bda06a4 to e791a00CompareJanuary 4, 2021 15:04
@dg
dgforce-pushed the master branch 4 times, most recently from f7f33af to 94c20b7CompareJanuary 21, 2021 14:56
@dg
dgforce-pushed the master branch 5 times, most recently from fd982d7 to be6a248CompareMarch 2, 2021 01:00
@mabar

Copy link
Copy Markdown
ContributorAuthor

@dg Would this change be acceptable for v4.0? Response will not sent headers directly anyway and it would be helpful if it didn't interfere with other http libraries.

Sign up for freeto 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.

2 participants

@mabar@dg