Hi internals,
Almost a year ago I first proposed my RFC draft to introduce a new
json_encode parameter 'indent'. I have received a lot of feedback on the
change, very insightful. The feedback can be boiled down to:
- Accepting user input characters means you could create invalid JSON.
Do we want that? Should it be complying with the spec[1]? - Preference for pure types, so int OR string, not both.
So I think I made the change more complex than it should have been and
considered the three options:
- Accept indent as an int, which will result in N spaces of indent
per indentation level. - Accept indent as a string, which will result in string N per
indentation level. - Accept indent as an int and indent_char as string, which will
result in N * indent_char per indentation level.
Option 1 seems very simple and feasible while not being confusing.
Option 2 seems feasible, but somewhat more complex, because user input
should be validated.
Option 3 seems very flexible, but in my opinion very confusing at the
same time, while I'm not sure there's even a use case for this level of
flexibility.
I have updated the pull request[2] and RFC[3] to be based on option 1,
as I think this offers clear functionality and I feel like I can't
really go wrong with the indent parameter as an int.
Please let me know what your thoughts are and what needs to be done to
get this RFC going forward!
--
Kind regards,
Timon
[1] https://datatracker.ietf.org/doc/html/rfc4627
[2] https://github.com/php/php-src/pull/7093
[3] https://wiki.php.net/rfc/json_encode_indentation
Hi,
Hi internals,
Almost a year ago I first proposed my RFC draft to introduce a new
json_encode parameter 'indent'. I have received a lot of feedback on the
change, very insightful. The feedback can be boiled down to:
- Accepting user input characters means you could create invalid JSON.
Do we want that? Should it be complying with the spec[1]?- Preference for pure types, so int OR string, not both.
So I think I made the change more complex than it should have been and
considered the three options:
- Accept indent as an int, which will result in N spaces of indent
per indentation level.- Accept indent as a string, which will result in string N per
indentation level.- Accept indent as an int and indent_char as string, which will
result in N * indent_char per indentation level.Option 1 seems very simple and feasible while not being confusing.
Option 2 seems feasible, but somewhat more complex, because user input
should be validated.
Option 3 seems very flexible, but in my opinion very confusing at the
same time, while I'm not sure there's even a use case for this level of
flexibility.I have updated the pull request[2] and RFC[3] to be based on option 1,
as I think this offers clear functionality and I feel like I can't
really go wrong with the indent parameter as an int.Please let me know what your thoughts are and what needs to be done to
get this RFC going forward!
I think we can put this RFC to the vote. If the author is to busy I would
like to start voting later this week. It would be a pity not to make it to
feature freeze as it is quite straight forward and the implementation seems
good as well so I guess we don't need to wait extra year. :)
Cheers
Jakub
I think we can put this RFC to the vote. If the author is to busy I would
like to start voting later this week. It would be a pity not to make it to
feature freeze as it is quite straight forward and the implementation seems
good as well so I guess we don't need to wait extra year. :)Cheers
Jakub
This RFC really should have included an option to use tabs instead of
spaces, which is, IMO, even more important than having a parameter for
the number of spaces. This could take the form of either
(a) a magic indent value such as -1, or
(b) a new flag (suggested name: JSON_INDENT_USING_TABS).
There should be no need to support indenting using multiple tabs per
level; for option (b), the value of the indent parameter should be ignored.
An indent-using-tabs flag would even be useful for generating two-space
indented JSON, should the proposed indent parameter be rejected. The
reason is that tabs cannot occur within JSON string representations, so
any other indent string is a simple str_replace() away. In contrast,
replacing the current four-space indent with anything else is more
complicated to do efficiently1.
As a flag can't really hurt, I'm thinking of going with option B before
starting the vote.
Also agree with ignoring the indent parameter, as a tab character is
less dependent on how much it is repeated.
Kind regards,
Timon
I think we can put this RFC to the vote. If the author is to busy I
would
like to start voting later this week. It would be a pity not to make
it to
feature freeze as it is quite straight forward and the implementation
seems
good as well so I guess we don't need to wait extra year. :)Cheers
Jakub
This RFC really should have included an option to use tabs instead of
spaces, which is, IMO, even more important than having a parameter for
the number of spaces. This could take the form of either(a) a magic indent value such as -1, or
(b) a new flag (suggested name: JSON_INDENT_USING_TABS).
There should be no need to support indenting using multiple tabs per
level; for option (b), the value of the indent parameter should be ignored.An indent-using-tabs flag would even be useful for generating two-space
indented JSON, should the proposed indent parameter be rejected. The
reason is that tabs cannot occur within JSON string representations, so
any other indent string is a simplestr_replace()away. In contrast,
replacing the current four-space indent with anything else is more
complicated to do efficiently1.
Hi internals,
If the rest also thinks the RFC is good to go, I suggest we start a vote
coming week.
As this is my first RFC, I'm not so sure how this typically gets kicked
off, so I'd like to know what I need to do!
Kind regards,
Timon
Hi,
On Fri, May 13, 2022 at 2:33 PM Timon de Groot <tdegroot96@gmail.com
mailto:tdegroot96@gmail.com> wrote:Hi internals, Almost a year ago I first proposed my RFC draft to introduce a new json_encode parameter 'indent'. I have received a lot of feedback on the change, very insightful. The feedback can be boiled down to: - Accepting user input characters means you could create invalid JSON. Do we want that? Should it be complying with the spec[1]? - Preference for pure types, so int OR string, not both. So I think I made the change more complex than it should have been and considered the three options: 1) Accept indent as an int, which will result in N spaces of indent per indentation level. 2) Accept indent as a string, which will result in string N per indentation level. 3) Accept indent as an int and indent_char as string, which will result in N * indent_char per indentation level. Option 1 seems very simple and feasible while not being confusing. Option 2 seems feasible, but somewhat more complex, because user input should be validated. Option 3 seems very flexible, but in my opinion very confusing at the same time, while I'm not sure there's even a use case for this level of flexibility. I have updated the pull request[2] and RFC[3] to be based on option 1, as I think this offers clear functionality and I feel like I can't really go wrong with the indent parameter as an int. Please let me know what your thoughts are and what needs to be done to get this RFC going forward!I think we can put this RFC to the vote. If the author is to busy I
would like to start voting later this week. It would be a pity not to
make it to feature freeze as it is quite straight forward and the
implementation seems good as well so I guess we don't need to wait extra
year. :)Cheers
Jakub
Hi
If the rest also thinks the RFC is good to go, I suggest we start a vote
coming week.
With the minimum voting period being 14 days as per
https://wiki.php.net/RFC/voting#voting and the feature freeze for PHP
8.2 being 2022-07-19 23:59:59 UTC as per
https://externals.io/message/118025#118028, starting the vote in the
coming week is too late. The vote needs to start no later than July, 5th
23:59 UTC (i.e. tomorrow).
Best regards
Tim Düsterhus
Hi internals,
If the rest also thinks the RFC is good to go, I suggest we start a vote
coming week.
As this is my first RFC, I'm not so sure how this typically gets kicked
off, so I'd like to know what I need to do!
You just need to change status in RFC to Voting and in voting section set
the date range and add doodle poll - you can basically copy it from one of
the open RFC's (see for example code in
https://wiki.php.net/rfc/fetch_property_in_const_expressions ). Then you
just need to send email to internals with subject prefixed with [RFC][VOTE]
or similar. As noted you need to do it today or latest tomorrow. Feel free
to let me know if you are too busy or something is not clear.
I would recommend not to do any last time changes as in such case you
should probably give an extra time for dicusion which means it won't make
the feature freeze and will have to wait for another year. In my opinion it
is good as it is. The tabs can be later added as an extra flag. If the flag
is set, we could just change default value for indent to 1 and use tabs but
it would be still possible for users to set indent size if they wish. I
think that's something that makes most sense to me and doesn't impact the
current RFC in any way.
Regards
Jakub
On Mon, Jul 4, 2022 at 8:38 AM Timon de Groot tdegroot96@gmail.com
wrote:Hi internals,
If the rest also thinks the RFC is good to go, I suggest we start a vote
coming week.
As this is my first RFC, I'm not so sure how this typically gets kicked
off, so I'd like to know what I need to do!You just need to change status in RFC to Voting and in voting section set
the date range and add doodle poll - you can basically copy it from one of
the open RFC's (see for example code in
https://wiki.php.net/rfc/fetch_property_in_const_expressions ). Then you
just need to send email to internals with subject prefixed with [RFC][VOTE]
or similar. As noted you need to do it today or latest tomorrow. Feel free
to let me know if you are too busy or something is not clear.I would recommend not to do any last time changes as in such case you
should probably give an extra time for dicusion which means it won't make
the feature freeze and will have to wait for another year. In my opinion it
is good as it is. The tabs can be later added as an extra flag. If the flag
is set, we could just change default value for indent to 1 and use tabs but
it would be still possible for users to set indent size if they wish. I
think that's something that makes most sense to me and doesn't impact the
current RFC in any way.Regards
Jakub
My thoughts might be firmly in the realms of "too little, too late" since
the vote is open already, so by all means choose to ignore.
Things that I would have liked to have seen in the RFC document:
- the mention/consideration of a user-specified indent string
(even if under a "Rejected Features" section with some details about the
rejection) - details on the new parameter's interaction with / dependency on
JSON_PRETTY_PRINT. - related, details on what happens when the new parameter is used without
JSON_PRETTY_PRINT.
(I'd personally like to haveJSON_PRETTY_PRINTbe implicitly added if the
new parameter is used, or it could
raise an error, or it could be ignored as seems to be the case but isn't
explicltly mentioned) - details on allowed values of the new parameter, e.g. what happens with
negative, or stupidly huge, values? - some summary (more than none!) of discussion topics / design decisions
resulting from the mailing list thread(s)
The RFC currently states (the quote is two-thirds of the proposal text,
minus examples):
By default, an indentation of 4 spaces will be applied, just like the
original json_encode behaviour with theJSON_PRETTY_PRINToption.When the indent parameter is passed a different value, an indentation of
N spaces will be applied.
Even after several readings of the proposal, I thought that you were
proposing that json_encode() always be pretty-printed and indented.
Even the examples weren't particularly helpful in correcting
that mis-reading. It took me far too long to understand (hopefully
correctly) that:
- "By default..." means "When the flags include
JSON_PRETTY_PRINTand the
indent parameter is not passed (or 4 is passed)...". - "When the indent parameter is passed..." really means "When the flags
includeJSON_PRETTY_PRINTand the indent parameter is passed...". - Any calls to
json_encode()without JSON_PRETTY_PRINT, with or without the
indent parameter, behave identically to now.
The "Unaffected PHP Functionality" section only muddied the waters further:
"Normal usage ... of the json_encode function will not be affected, as
the default of 4 spaces will still be in effect."
My point being, I don't think this document was anywhere near ready... but
feel free to disagree.
Just to finish up, wouldn't it be nice to have the following?..
json_encode($value, indent: 2)
Regards,
Peter
Hey all.
On Mon, Jul 4, 2022 at 8:38 AM Timon de Groot tdegroot96@gmail.com
wrote:Hi internals,
If the rest also thinks the RFC is good to go, I suggest we start a vote
coming week.
As this is my first RFC, I'm not so sure how this typically gets kicked
off, so I'd like to know what I need to do!You just need to change status in RFC to Voting and in voting section set
the date range and add doodle poll - you can basically copy it from one of
the open RFC's (see for example code in
https://wiki.php.net/rfc/fetch_property_in_const_expressions ). Then you
just need to send email to internals with subject prefixed with [RFC][VOTE]
or similar. As noted you need to do it today or latest tomorrow. Feel free
to let me know if you are too busy or something is not clear.I would recommend not to do any last time changes as in such case you
should probably give an extra time for dicusion which means it won't make
the feature freeze and will have to wait for another year. In my opinion it
is good as it is. The tabs can be later added as an extra flag. If the flag
is set, we could just change default value for indent to 1 and use tabs but
it would be still possible for users to set indent size if they wish. I
think that's something that makes most sense to me and doesn't impact the
current RFC in any way.Regards
Jakub
My thoughts might be firmly in the realms of "too little, too late" since
the vote is open already, so by all means choose to ignore.
I'll be on the same lines!
What I would have prefered is instead of a the new parameter $indent
having a numerical value to have a string value.
That would have allowed the following:
json_encode($data, indent: " ");
or
json_encode($data, indent: "\t");
That would have made the tabs vs. spaces very easy and people can also
add whatever they want to set for one indentation. Think about setting
dots for making the spaces visible for some formatted output or whatever
else people might think of.
And setting the "indent" parameter would always implicitly set the flag
"JSON_PRETTY_PRINT" (as it doesn't make sense to not have that set).
The current way of just being able to set a number of spaces to indent
or to use a flag for tabs is a bit fiddly at best and does make adding
accessibility a second class citizen. Therefore I will vote against this
current implementation.
Cheers
Andreas
,,,
(o o)
+---------------------------------------------------------ooO-(_)-Ooo-+
| Andreas Heigl |
| mailto:andreas@heigl.org N 50°22'59.5" E 08°23'58" |
| https://andreas.heigl.org |
+---------------------------------------------------------------------+
| https://hei.gl/appointmentwithandreas |
+---------------------------------------------------------------------+
What I would have prefered is instead of a the new parameter $indent
having a numerical value to have a string value.That would have allowed the following:
json_encode($data, indent: " ");
or
json_encode($data, indent: "\t");
That would have made the tabs vs. spaces very easy and people can also
add whatever they want to set for one indentation. Think about setting
dots for making the spaces visible for some formatted output or whatever
else people might think of.And setting the "indent" parameter would always implicitly set the flag
"JSON_PRETTY_PRINT" (as it doesn't make sense to not have that set).The current way of just being able to set a number of spaces to indent
or to use a flag for tabs is a bit fiddly at best and does make adding
accessibility a second class citizen. Therefore I will vote against this
current implementation.
I voted No for the same reason.
--
Aleksander Machniak
Kolab Groupware Developer [https://kolab.org]
Roundcube Webmail Developer [https://roundcube.net]
PGP: 19359DC1 # Blog: https://kolabian.wordpress.com
Hey all.
On Mon, Jul 4, 2022 at 8:38 AM Timon de Groot tdegroot96@gmail.com
wrote:Hi internals,
If the rest also thinks the RFC is good to go, I suggest we start a
vote
coming week.
As this is my first RFC, I'm not so sure how this typically gets kicked
off, so I'd like to know what I need to do!You just need to change status in RFC to Voting and in voting section
set
the date range and add doodle poll - you can basically copy it from one
of
the open RFC's (see for example code in
https://wiki.php.net/rfc/fetch_property_in_const_expressions ). Then
you
just need to send email to internals with subject prefixed with
[RFC][VOTE]
or similar. As noted you need to do it today or latest tomorrow. Feel
free
to let me know if you are too busy or something is not clear.I would recommend not to do any last time changes as in such case you
should probably give an extra time for dicusion which means it won't
make
the feature freeze and will have to wait for another year. In my
opinion it
is good as it is. The tabs can be later added as an extra flag. If the
flag
is set, we could just change default value for indent to 1 and use tabs
but
it would be still possible for users to set indent size if they wish. I
think that's something that makes most sense to me and doesn't impact
the
current RFC in any way.Regards
Jakub
My thoughts might be firmly in the realms of "too little, too late" since
the vote is open already, so by all means choose to ignore.I'll be on the same lines!
What I would have prefered is instead of a the new parameter $indent
having a numerical value to have a string value.That would have allowed the following:
json_encode($data, indent: " ");or
json_encode($data, indent: "\t");That would have made the tabs vs. spaces very easy and people can also
add whatever they want to set for one indentation. Think about setting
dots for making the spaces visible for some formatted output or whatever
else people might think of.
The problem with this approach is that we either need to validate it (which
is probably not a big deal but you still have just option or using spaces
or tabs) or we allow invalid JSON to be produced from this function which I
think it's not a good idea and should not be part of json_encode because,
as its name suggests, that function is just for encoding JSON and not some
random format. My guess is that it would be even less likely to pass and I
would certainly vote against it.
I think if this RFC fails, we could maybe allow number or validated string
and also do that auto setting of flag if set as suggested.
Regards
Jakub
Hey all.
On Mon, Jul 4, 2022 at 8:38 AM Timon de Groot tdegroot96@gmail.com
wrote:Hi internals,
If the rest also thinks the RFC is good to go, I suggest we start a
vote
coming week.
As this is my first RFC, I'm not so sure how this typically gets kicked
off, so I'd like to know what I need to do!You just need to change status in RFC to Voting and in voting section
set
the date range and add doodle poll - you can basically copy it from one
of
the open RFC's (see for example code in
https://wiki.php.net/rfc/fetch_property_in_const_expressions ). Then
you
just need to send email to internals with subject prefixed with
[RFC][VOTE]
or similar. As noted you need to do it today or latest tomorrow. Feel
free
to let me know if you are too busy or something is not clear.I would recommend not to do any last time changes as in such case you
should probably give an extra time for dicusion which means it won't
make
the feature freeze and will have to wait for another year. In my
opinion it
is good as it is. The tabs can be later added as an extra flag. If the
flag
is set, we could just change default value for indent to 1 and use tabs
but
it would be still possible for users to set indent size if they wish. I
think that's something that makes most sense to me and doesn't impact
the
current RFC in any way.Regards
Jakub
My thoughts might be firmly in the realms of "too little, too late" since
the vote is open already, so by all means choose to ignore.I'll be on the same lines!
What I would have prefered is instead of a the new parameter $indent
having a numerical value to have a string value.That would have allowed the following:
json_encode($data, indent: " ");or
json_encode($data, indent: "\t");That would have made the tabs vs. spaces very easy and people can also
add whatever they want to set for one indentation. Think about setting
dots for making the spaces visible for some formatted output or whatever
else people might think of.The problem with this approach is that we either need to validate it (which
is probably not a big deal but you still have just option or using spaces
or tabs) or we allow invalid JSON to be produced from this function which I
think it's not a good idea and should not be part of json_encode because,
as its name suggests, that function is just for encoding JSON and not some
random format. My guess is that it would be even less likely to pass and I
would certainly vote against it.I think if this RFC fails, we could maybe allow number or validated string
and also do that auto setting of flag if set as suggested.
Then perhaps adding a separate function "json_format" would be the
better approach?
Keep json_encode to only encode json into a "binary" format (and
then it's irrelevant whether that's spaces or tabs) and explicitly use a
different function to handle possible formatting topics with different
spaces, tabs whatever-else-one-might-find-adequate....
Cheers
Andreas
--
,,,
(o o)
+---------------------------------------------------------ooO-(_)-Ooo-+
| Andreas Heigl |
| mailto:andreas@heigl.org N 50°22'59.5" E 08°23'58" |
| https://andreas.heigl.org |
+---------------------------------------------------------------------+
| https://hei.gl/appointmentwithandreas |
+---------------------------------------------------------------------+
Hey Timon,
Hi internals,
Almost a year ago I first proposed my RFC draft to introduce a new
json_encode parameter 'indent'. I have received a lot of feedback on the
change, very insightful. The feedback can be boiled down to:
- Accepting user input characters means you could create invalid JSON.
Do we want that? Should it be complying with the spec[1]?- Preference for pure types, so int OR string, not both.
So I think I made the change more complex than it should have been and
considered the three options:
- Accept indent as an int, which will result in N spaces of indent
per indentation level.- Accept indent as a string, which will result in string N per
indentation level.- Accept indent as an int and indent_char as string, which will
result in N * indent_char per indentation level.Option 1 seems very simple and feasible while not being confusing.
Option 2 seems feasible, but somewhat more complex, because user input
should be validated.
Option 3 seems very flexible, but in my opinion very confusing at the
same time, while I'm not sure there's even a use case for this level of
flexibility.I have updated the pull request[2] and RFC[3] to be based on option 1,
as I think this offers clear functionality and I feel like I can't
really go wrong with the indent parameter as an int.Please let me know what your thoughts are and what needs to be done to
get this RFC going forward!--
Kind regards,
Timon[1] https://datatracker.ietf.org/doc/html/rfc4627
[2] https://github.com/php/php-src/pull/7093
[3] https://wiki.php.net/rfc/json_encode_indentation--
To unsubscribe, visit: https://www.php.net/unsub.php
I went with "NO" here for various reasons:
- usage of
JSON_PRETTY_PRINTis already rare enough to make the
relevance of this parameter almost null - complicates an API signature for a very tiny detail
- if the problem is linting/preferences, have your favorite linter
process the file in the way you deem most appropriate
Doesn't need to live in php-src.
Greets,
Marco Pivetta