Skip to content

improve headers_list() example#1007

Merged
kamil-tekiela merged 1 commit intophp:masterfrom
hakre:patch-1
Oct 12, 2021
Merged

improve headers_list() example#1007
kamil-tekiela merged 1 commit intophp:masterfrom
hakre:patch-1

Conversation

@hakre
Copy link
Copy Markdown
Contributor

@hakre hakre commented Oct 10, 2021

Two things with the PHP example code:

  1. X- prefix header names June 2012 1, 2.

  2. PHP prepends the default_charset 3 on any text/* media type in Content-Type header (unless the charset is with the header() call. The example code output now has it (with the UTF-8 default).

@hakre hakre force-pushed the patch-1 branch 2 times, most recently from 3da7c7a to 029df69 Compare October 10, 2021 21:11
Copy link
Copy Markdown
Member

@tiffany-taylor tiffany-taylor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These lines

[0]=>
string(23) "X-Powered-By: PHP/5.1.3"
could probably be removed and updated to [0].

@hakre hakre force-pushed the patch-1 branch 3 times, most recently from 4f3fa6f to 862940b Compare October 10, 2021 22:22
Copy link
Copy Markdown
Contributor

@salathe salathe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @hakre !

Added a few details that need to be fixed, as suggested changes.

Other things to consider changing, though not required:

  • Change &example.outputs; to &examples.outputs.similar; (i.e. to say The above examples will output something similar to: ) since the output may vary by PHP version, configuration, SAPI (?), etc. These XML entities, and many others, can be found in the language-snippets.ent file in the top-level directory of the repo.
  • Update, or remove for simplicity, the X-Powered-By header. PHP 5.1.3 is pretty ancient. Edit: it looks like you updated the version number while I was typing this. :)

@hakre hakre force-pushed the patch-1 branch 2 times, most recently from 3395f29 to ea085a3 Compare October 10, 2021 22:29
@hakre
Copy link
Copy Markdown
Contributor Author

hakre commented Oct 10, 2021

There seems to be some non-signigifcant whitespace issue as well, I'll add it.

@kamil-tekiela
Copy link
Copy Markdown
Member

I think I preferred your first version where you added charset. The header produced by PHP is nonconformant from what I see.

@hakre
Copy link
Copy Markdown
Contributor Author

hakre commented Oct 10, 2021

I think I preferred your first version where you added charset. The header produced by PHP is nonconformant from what I see.

Not sure I get what you mean. The header produced by PHP looks fine, doesn't it?:

Content-type: text/plain;charset=UTF-8

HTTP Header name fine, mime media type fine, charset attribute fine. not? @kamil-tekiela

@salathe salathe requested review from salathe and removed request for salathe October 10, 2021 22:40
@kamil-tekiela
Copy link
Copy Markdown
Member

kamil-tekiela commented Oct 10, 2021

When you specify the charset explicitely then PHP will produce the header exactly as specified:

Content-Type: text/html; charset=UTF-8

When you omit the charset, PHP will lowercase type and remove space before ;:

Content-type: text/html;charset=UTF-8

The example given by MDN is

Content-Type: text/html; charset=UTF-8

@hakre
Copy link
Copy Markdown
Contributor Author

hakre commented Oct 10, 2021

@kamil-tekiela: The whitespace is optional (OWS - optional white space)

RFC 7231 Section 3.1.1.1 says:

media-type = type "/" subtype *( OWS ";" OWS parameter )

and gives the following examples for media types in the Content-Type header:

     text/html;charset=utf-8
     text/html;charset=UTF-8
     Text/HTML;Charset="utf-8"
     text/html; charset="utf-8"

The second and the last one come close to the one on MDN but actually the example on MDN is not in there.. I'd say it is another valid example.

@kamil-tekiela
Copy link
Copy Markdown
Member

Thanks for pointing to the spec. I am aware the spaces are optional and the type, subtype and parameter are case-insensitive. I am not sure about the header name, but as I see user agents treat it in case insensitive way.

My point is that when we document the output header, it would be safer to use the variant where PHP doesn't change the case or white space. What if we decide to change the Content-type to Content-Type in a future PHP release? If it's possible to document in such a way that we can be sure of what the output is without relying on the way PHP prepares it internally, or some INI directive, then I think that would be preferable.

@hakre
Copy link
Copy Markdown
Contributor Author

hakre commented Oct 10, 2021

My point is that when we document the output header, it would be safer to use the variant where PHP doesn't change the case or white space. What if we decide to change the Content-type to Content-Type in a future PHP release?

Well it's docs and the example output is taken from the PHP version as in the X-Powered-By header. No decision for changes in src from my end, the examples should not imply anything regarding that from my perspective.

Similar to the optional white space, case does not matter here (the header names are equivalent, similar to the token and quoted token in the media type). For the header names that is in the HTTP specs, MDN should have it as well as IETF.

Therefore I don't see an issue with that. Also &examples.outputs.similar; has been edited in.

Additional ref regarding case-sensitivity: RFC 4790 9.2.1. ASCII Casemap Collation Description (non-locale-aware, Posix "C" locale like)

But perhaps too technical for a docs discussion maybe @kamil-tekiela

@kamil-tekiela
Copy link
Copy Markdown
Member

Sorry, maybe my language skills are at fault here. I am not questioning the RFC or why PHP outputs the header the way it does. All I am saying is that it's a little confusing for readers of the PHP manual to see that you are sending Content-Type but the output example says Content-type. This could be easily avoided if you send the header in full with the charset. This way, the case in the example's code and the example's output will match. Also, specifying the charset in the header in code, will not leave readers wondering where the charset came from.

I am not sure how the discussion became technical; I did not mean to make it technical. All I said is that the example looked better when you had:

/* Specify plain text content in our response */
header('Content-Type: text/plain; charset=UTF-8');

The output will then be exactly what you provided to the header() function.
If I am still not making sense you can ping me in chat in room 11, maybe I will be able to explain it better.

@hakre
Copy link
Copy Markdown
Contributor Author

hakre commented Oct 10, 2021

Modified to Content-Type: text/plain; charset=UTF-8.

Copy link
Copy Markdown
Member

@kamil-tekiela kamil-tekiela left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Other than this one small change it looks good.

hakre added a commit to hakre/doc-en that referenced this pull request Oct 11, 2021
Two & 1/2 things with the PHP example code:

1. `X-` prefix header names June 2012 [1], [2].

2. PHP prepends the `default_charset` [3] on any `text/*` media type in
   Content-Type header, unless the `charset` is with the `header()` call,
   like in the example.

3. PHP version 5.1.3 (May 2006) -> 8.0.11, the X-Powered-By header [4]
   removed (only distracting w/ the example).

PHP version in use to generate the output: 8.0.11.

[1]: https://datatracker.ietf.org/doc/html/rfc6648
[2]: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers
[3]: https://www.php.net/manual/en/ini.core.php#ini.default-charset
[4]: https://www.php.net/manual/en/ini.core.php#ini.expose-php

Co-authored-by: Tiffany <tiffany.k.taylor@gmail.com>
hakre added a commit to hakre/doc-en that referenced this pull request Oct 12, 2021
Two & 1/2 things with the PHP example code:

1. `X-` prefix header names June 2012 [1], [2].

2. PHP prepends the `default_charset` [3] on any `text/*` media type in
   Content-Type header, unless the `charset` is with the `header()` call,
   like in the example.

3. PHP version 5.1.3 (May 2006) -> 8.0.11, the X-Powered-By header [4]
   removed (only distracting w/ the example).

PHP version in use to generate the output: 8.0.11.

[1]: https://datatracker.ietf.org/doc/html/rfc6648
[2]: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers
[3]: https://www.php.net/manual/en/ini.core.php#ini.default-charset
[4]: https://www.php.net/manual/en/ini.core.php#ini.expose-php

Co-authored-by: Tiffany <tiffany.k.taylor@gmail.com>
Two & 1/2 things with the PHP example code:

1. `X-` prefix header names June 2012 [1], [2].

2. PHP prepends the `default_charset` [3] on any `text/*` media type in
   Content-Type header, unless the `charset` is with the `header()` call,
   like in the example.

3. PHP version 5.1.3 (May 2006) -> 8.0.11, the X-Powered-By header [4]
   removed (only distracting w/ the example).

PHP version in use to generate the output: 8.0.11.

[1]: https://datatracker.ietf.org/doc/html/rfc6648
[2]: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers
[3]: https://www.php.net/manual/en/ini.core.php#ini.default-charset
[4]: https://www.php.net/manual/en/ini.core.php#ini.expose-php

Co-authored-by: Tiffany <tiffany.k.taylor@gmail.com>
@kamil-tekiela kamil-tekiela merged commit eecf090 into php:master Oct 12, 2021
@kamil-tekiela
Copy link
Copy Markdown
Member

Thank you. I merged it now.

@hakre hakre deleted the patch-1 branch October 12, 2021 13:05
marcovtwout pushed a commit to marcovtwout/doc-en that referenced this pull request Dec 24, 2024
Two & 1/2 things with the PHP example code:

1. `X-` prefix header names June 2012 [1], [2].

2. PHP prepends the `default_charset` [3] on any `text/*` media type in
   Content-Type header, unless the `charset` is with the `header()` call,
   like in the example.

3. PHP version 5.1.3 (May 2006) -> 8.0.11, the X-Powered-By header [4]
   removed (only distracting w/ the example).

PHP version in use to generate the output: 8.0.11.

[1]: https://datatracker.ietf.org/doc/html/rfc6648
[2]: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers
[3]: https://www.php.net/manual/en/ini.core.php#ini.default-charset
[4]: https://www.php.net/manual/en/ini.core.php#ini.expose-php

Co-authored-by: Tiffany <tiffany.k.taylor@gmail.com>
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.

4 participants