Everything

php

Home » Project » Newsgroups » php.internals » [RFC] Add json_encode indent parameter »

[RFC] Add json_encode indent parameter

php.internals

Timon de Groot

4 years ago

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!

--

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

Jakub Zelenka

4 years ago

Hi,

On Fri, May 13, 2022 at 2:33 PM Timon de Groot <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

Kevin Israel

4 years ago

On 7/3/22 18:01, Jakub Zelenka wrote:

>   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 efficiently[1].

[1]:
https://github.com/wikimedia/mediawiki/blob/210a34369ac8f0ba74b497d0b2298ca7e5a0bffb/includes/json/FormatJson.php#L113-L117

Timon de Groot

4 years ago

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

On 04-07-2022 03:22, Kevin Israel wrote:

Timon de Groot

4 years ago

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

On 04-07-2022 00:01, Jakub Zelenka wrote:

Tim Düsterhus

4 years ago

Hi

On 7/4/22 09:38, Timon de Groot wrote:

> 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

Jakub Zelenka

4 years ago

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

Peter Cowburn

4 years ago

On Mon, 4 Jul 2022 at 11:11, Jakub Zelenka <bukka@php.net> wrote:

> 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 have JSON_PRETTY_PRINT be 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 the JSON_PRETTY_PRINT option.

>
> 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_PRINT and the
indent parameter is not passed (or 4 is passed)...".
* "When the indent parameter is passed..." really means "When the flags
include JSON_PRETTY_PRINT and 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

Andreas Heigl

4 years ago

Hey all.

On 05.07.22 02:04, Peter Cowburn wrote:

> On Mon, 4 Jul 2022 at 11:11, Jakub Zelenka <bukka@php.net> wrote:
>
>> 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                               |
+---------------------------------------------------------------------+

A.L.E.C

4 years ago

On 05.07.2022 09:37, Andreas Heigl wrote:

> 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

Jakub Zelenka

4 years ago

On Tue, Jul 5, 2022 at 8:37 AM Andreas Heigl <andreas@heigl.org> wrote:

> Hey all.
>
> On 05.07.22 02:04, Peter Cowburn wrote:
> > On Mon, 4 Jul 2022 at 11:11, Jakub Zelenka <bukka@php.net> wrote:
> >
> >> 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

Andreas Heigl

4 years ago

On 05.07.22 11:38, Jakub Zelenka wrote:

> On Tue, Jul 5, 2022 at 8:37 AM Andreas Heigl <andreas@heigl.org> wrote:
>
>> Hey all.
>>
>> On 05.07.22 02:04, Peter Cowburn wrote:
>>> On Mon, 4 Jul 2022 at 11:11, Jakub Zelenka <bukka@php.net> wrote:
>>>
>>>> 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                               |
+---------------------------------------------------------------------+

Marco Pivetta

4 years ago

Hey Timon,

On Fri, 13 May 2022 at 15:33, Timon de Groot <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!
>
> --
>
> 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
>
> --
> PHP Internals - PHP Runtime Development Mailing List
> To unsubscribe, visit: https://www.php.net/unsub.php

I went with "NO" here for various reasons:

1. usage of `JSON_PRETTY_PRINT` is already rare enough to make the
relevance of this parameter almost null
2. complicates an API signature for a very tiny detail
3. 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

https://twitter.com/Ocramius

https://ocramius.github.io/