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

Incorrect documentation for {tls_version} placeholder #2146

Open
vcsjones opened this Issue Apr 29, 2018 · 7 comments

Comments

4 participants
@vcsjones

vcsjones commented Apr 29, 2018

(Logging issue per a forum topic):

While setting up logging, I believe I found an issue with the documentation issue for the TLS version placeholder. The documentation for placeholders indicates that the placeholder is called {tls_version}. However, this placeholder always produces a value of "-".

Looking at the source, it appears that the placeholder is actually called {tls_protocol}:

case "{tls_protocol}":

@mholt mholt added the documentation label Apr 29, 2018

@mholt

This comment has been minimized.

Owner

mholt commented Apr 29, 2018

Oops, you are right! Will fix that soon. Thanks.

@detroitenglish

This comment has been minimized.

detroitenglish commented Jun 30, 2018

Fell victim to this today trying to rewrite clients using tls1.0 ... thought I was going bananas 🍌🙈

@mholt

This comment has been minimized.

Owner

mholt commented Jul 9, 2018

I wonder if we should change it to tls_version. Wouldn't that make more sense?

@vcsjones

This comment has been minimized.

vcsjones commented Jul 9, 2018

Wouldn't that make more sense?

It would make more sense long term yes, at the cost of this being a breaking change that won't break in an obvious way (our logger would just start logging "-" until someone noticed it).

I'm not sure how Caddy handles deprecation, but perhaps:

case "{tls_protocol}":
    // Warn deprecated and will be removed in the future.
    fallthrough;
case "{tls_version}":
    //etc
@francislavoie

This comment has been minimized.

Collaborator

francislavoie commented Jul 9, 2018

+1 for deprecation and potentially removal in 0.12-ish, whenever that happens

@mholt

This comment has been minimized.

Owner

mholt commented Jul 9, 2018

Yeah, we can do that, although the docs always said tls_version so this is more of a bug fix (a patch release) than a breaking change.

+1 for deprecation and potentially removal in 0.12-ish, whenever that happens

Hoping not to have a 0.12 tree; next major release should be 1.0.

@francislavoie

This comment has been minimized.

Collaborator

francislavoie commented Jul 9, 2018

Okay 😛 I was just going by the milestones listed in github

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment