Skip to content

improve: supported the link response header #1459

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

Closed
wants to merge 1 commit into from

Conversation

tokers
Copy link
Contributor

@tokers tokers commented Jan 22, 2019

Since Nginx/1.13.10. The Link header was treated as a common header and
equipped with a shortcut pointer (r->headers_out.link). Currently this module doesn't
follow this behavior. Consequently, Link header set by this module
(e.g. in header_filter_by_lua*) cannot be referenced from
r->headers_out.link. It may cause some inconsistent problems. For instance,
the HTTP/2 server push preload option, see #1458 for details.

This Commit placed the Link header inside ngx_http_lua_set_handlers[].

I hereby granted the copyright of the changes in this pull request
to the authors of this lua-nginx-module project.

Since Nginx/1.13.10. The `Link` header was treated as a common header and
equipped a shortcut pointer (`r->headers_out.link`). Currently this module doesn't
follow this behavior. Consequently, `Link` header set by this module
(e.g. in `header_filter_by_lua*`) cannot be referenced from
`r->headers_out.link`.  It may cause some inconsistent problems. For instance,
the HTTP/2 server push preload, see openresty#1458 for details.

This Commit placed the Link header inside `ngx_http_lua_set_handlers[]`.
@thibaultcha
Copy link
Member

This seems like a duplicate of #1389 ? Although being on mobile right now, I have not double checked.

@tokers
Copy link
Contributor Author

tokers commented Jan 22, 2019

@thibaultcha
Oops. I didn't check this list before.

@tokers
Copy link
Contributor Author

tokers commented Jan 22, 2019

@thibaultcha I'm going to close this PR.

@tokers tokers closed this Jan 22, 2019
@tokers tokers deleted the improve/link-header-support branch January 22, 2019 03:59
@thibaultcha
Copy link
Member

thibaultcha commented Jan 22, 2019

The other PR has been lacking tests for a while now... Maybe yours could add them?

@tokers
Copy link
Contributor Author

tokers commented Jan 22, 2019

@thibaultcha
Well, I don't have the permission to push my changes to the other branch.

@thibaultcha
Copy link
Member

I meant into your own PR (this one). Another option would be a new PR with the existing commit and your own adding the tests.

@tokers
Copy link
Contributor Author

tokers commented Jan 22, 2019

@thibaultcha
Sorry I misunderstood your means ... Okey, I will add some test cases to cover this change.

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.

2 participants