Skip to content

Conversation

@elsloo
Copy link
Contributor

@elsloo elsloo commented Jun 3, 2021

This is a backport of changes made to the xdebug plugin via PR #7784 and it depends on backport PR #7919.

(cherry picked from commit 7433038)

@elsloo elsloo requested review from bryancall and zwoop as code owners June 3, 2021 22:15
@rob05c
Copy link
Member

rob05c commented Jun 3, 2021

I see this is a backport, I missed the original, sorry. But, the IETF has asked people not to use the X- prefix anymore.

Mostly because things tend to become standards, and then it's hard to remove the X-.

https://datatracker.ietf.org/doc/html/rfc6648

Deprecating the "X-" Prefix and Similar Constructs
in Application Protocols

Is it too late to change this to just Cache-Info?

@elsloo
Copy link
Contributor Author

elsloo commented Jun 3, 2021

I see this is a backport, I missed the original, sorry. But, the IETF has asked people not to use the X- prefix anymore.

Mostly because things tend to become standards, and then it's hard to remove the X-.

https://datatracker.ietf.org/doc/html/rfc6648

Deprecating the "X-" Prefix and Similar Constructs
in Application Protocols

Yes, I'm aware of this and chose consistency over following the best practice you referenced. X is used throughout the plugin, the headers, and its name. Having a single header without the prefix would lead to a less intuitive experience when using this plugin.

Is it too late to change this to just Cache-Info?

IMO, yes, this is a backport as you noted above.

@ezelkow1
Copy link
Member

ezelkow1 commented Jun 3, 2021

I am +1 (once we get #7919 in so this builds). This is a debug plugin, its not adding actionable headers to the core but in an optional debug plugin that is also easily changed if somehow any of these become standards. Plus its a backport anyway and it already exists in the code base for future versions

@ezelkow1 ezelkow1 added the Backport Marked for backport for an LTS patch release label Jun 4, 2021
@ywkaras
Copy link
Contributor

ywkaras commented Jun 8, 2021

It looks like #7783 will have to be backported to 9.1 before this?

@ezelkow1
Copy link
Member

ezelkow1 commented Jun 8, 2021

It looks like #7783 will have to be backported to 9.1 before this?

yea that backport is in 7919, thats why it fails CI for now

@elsloo
Copy link
Contributor Author

elsloo commented Jun 8, 2021

It looks like #7783 will have to be backported to 9.1 before this?

yea that backport is in 7919, thats why it fails CI for now

Correct, however none of it really matters because @zwoop will likely cherry-pick these manually into 9.1.x, at which point, both of the PRs will be closed.

@ywkaras
Copy link
Contributor

ywkaras commented Jun 8, 2021

I think then the project for #7783 needs to be changed to 9.1.

@zwoop zwoop closed this Jun 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Backport Marked for backport for an LTS patch release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants