Re: [PHP-DEV] Small annoyances of PHP: Simple fixes would make tests fail

[Guilliam Xavier]

On Fri, Mar 5, 2021 at 4:56 PM Nikita Popov  wrote:

> On Fri, Mar 5, 2021 at 1:36 PM Christian Schneider 
> wrote:
>
> > There is a PR fixing both the indentation and removing the trailing
> > spaces: https://github.com/php/php-src/pull/6724
> >
> > Do you think this needs an RFC?
> >
>
> Well ... making stylistic changes to var_export() has come up a few times
> in the past -- by far the most common request is to use [] instead of
> array(). I think if we're going to make changes here, we should at least
> include that one as well, to make sure all changes happen at the same time.
>
> I'm personally happy with modifying the var_export() output, but from past
> discussions, I got the impression that not everyone else feels that such
> cosmetic changes are justified.
>

On the other hand, the extra space in indentation for objects (vs arrays)
seems an outright bug in implementation, and the trailing space (after
double-arrow before newline) is a common source of problems when editing
.phpt files; whereas changing array() to [] would "just" make the output
shorter (and "more modern") while breaking more tests...

While at it, there are two more things that bug me:
  - the space before the opening parenthesis in "array (", vs
"::__set_state(array(" / "(object) array("
  - the line break after "=>" when the value is an array or object (which
is also the cause of the trailing space, by the way)
but changing them would be a bigger break, too...

Regards,

-- 
Guilliam Xavier

Re: [PHP-DEV] Small annoyances of PHP: Simple fixes would make tests fail

[Nikita Popov]

On Fri, Mar 5, 2021 at 1:36 PM Christian Schneider 
wrote:

> Am 19.02.2021 um 16:21 schrieb Sara Golemon :
> > On Fri, Feb 19, 2021 at 8:04 AM Christian Schneider <
> cschn...@cschneid.com > wrote:
> > Indentation in var_export:
> > 
> > The indentation for
> > var_export((object)[(object)[]]);
> > is off by one for nested structures:
> > (object) array(
> >'0' =>   # <-- extra space before index
> >   (object) array(
> > and the fix would be to change
> > buffer_append_spaces(buf, level + 2)
> > to
> > buffer_append_spaces(buf, level + 1)
> > in ext/standard/var.c
> >
> > This breaks about 60 PHP tests relying on the var_export format.
> >
> >
> > I think shifting the indentation in var_export() could probably be done
> without a major BC break.  Trimming trailing whitespace would also resolve
> one of my long-time annoyances with the function.
> > Feel free to propose that as a PR and/or an RFC.
>
> There is a PR fixing both the indentation and removing the trailing
> spaces: https://github.com/php/php-src/pull/6724
>
> Do you think this needs an RFC?
>

Well ... making stylistic changes to var_export() has come up a few times
in the past -- by far the most common request is to use [] instead of
array(). I think if we're going to make changes here, we should at least
include that one as well, to make sure all changes happen at the same time.

I'm personally happy with modifying the var_export() output, but from past
discussions, I got the impression that not everyone else feels that such
cosmetic changes are justified.

Nikita

Re: [PHP-DEV] Small annoyances of PHP: Simple fixes would make tests fail

[Christian Schneider]

Am 19.02.2021 um 16:21 schrieb Sara Golemon :
> On Fri, Feb 19, 2021 at 8:04 AM Christian Schneider  > wrote:
> Indentation in var_export:
> 
> The indentation for
> var_export((object)[(object)[]]);
> is off by one for nested structures:
> (object) array(
>'0' =>   # <-- extra space before index
>   (object) array(
> and the fix would be to change
> buffer_append_spaces(buf, level + 2)
> to
> buffer_append_spaces(buf, level + 1)
> in ext/standard/var.c
> 
> This breaks about 60 PHP tests relying on the var_export format.
> 
> 
> I think shifting the indentation in var_export() could probably be done 
> without a major BC break.  Trimming trailing whitespace would also resolve 
> one of my long-time annoyances with the function.
> Feel free to propose that as a PR and/or an RFC.

There is a PR fixing both the indentation and removing the trailing spaces: 
https://github.com/php/php-src/pull/6724

Do you think this needs an RFC?

- Chris

Re: [PHP-DEV] Small annoyances of PHP: Simple fixes would make tests fail

[Rowan Tommins]

On 22/02/2021 10:14, Christian Schneider wrote:

Side-note: Maybe we should recommend a function to be used for tests. At my 
work we used to use var_export(), some of our newer tests are using 
json_encode() but neither is really guaranteed to be stable, right?



Well, the obvious choice to me would be var_dump, which is used by the 
majority of PHP's internal tests (see 
https://externals.io/message/112967#112971 for some numbers). I was 
considering putting together a pull request that standardises on that 
for all bundled extensions, fixing the few hundred (out of thousands) 
that use var_export or print_r.


However, I've never used PHPT-style tests outside of PHP internals, and 
for anything like PHPUnit or PHPSpec you would normally be testing 
against actual PHP values, not any string representation; so I don't 
really have any opinion on that scenario.


Regards,

--
Rowan Tommins
[IMSoP]

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php

Re: [PHP-DEV] Small annoyances of PHP: Simple fixes would make tests fail

[Christian Schneider]

Am 19.02.2021 um 16:21 schrieb Sara Golemon :
> On Fri, Feb 19, 2021 at 8:04 AM Christian Schneider  > wrote:
> Stack traces:
> ==
> 
> What version are you testing with. I know Niki killed some unnecessary frames 
> in 8.0's stacktrace output. (A whole repro script is more useful than a 
> snippet wherein $e comes into being seemingly from nowhere).

I was testing with PHP 8.0.3 but it is the same with master.
You can see the difference at https://3v4l.org/0Pgfo  
where the first line
try { $a[(object)null] = 1; } catch (Throwable $e) { 
print_r($e->getTrace()); }
does not create a stack trace. That's what I'd like to change.

What do you think? Should I create a PR for this?
It does break a lot of tests in PHP. Probably less so for other projects but 
still...

> Indentation in var_export:
> 
> The indentation for
> var_export((object)[(object)[]]);
> is off by one for nested structures:
> (object) array(
>'0' =>   # <-- extra space before index
>   (object) array(
> and the fix would be to change
> buffer_append_spaces(buf, level + 2)
> to
> buffer_append_spaces(buf, level + 1)
> in ext/standard/var.c
> 
> This breaks about 60 PHP tests relying on the var_export format.
> 
> 
> I think shifting the indentation in var_export() could probably be done 
> without a major BC break.

An example script is at https://3v4l.org/V8r5k 
It does break tests though. And my guess it that is would also break tests in 
quite some projects.

What version would you aim this for? 8.1? 9.0?

> Trimming trailing whitespace would also resolve one of my long-time 
> annoyances with the function.
> Feel free to propose that as a PR and/or an RFC.

I see your point as it means there have to be trailing white-spaces in tests 
which can be confusing when editing/creating tests.
I can have a look at how hard this would be to fix...

Side-note: Maybe we should recommend a function to be used for tests. At my 
work we used to use var_export(), some of our newer tests are using 
json_encode() but neither is really guaranteed to be stable, right?

- Chris

Re: [PHP-DEV] Small annoyances of PHP: Simple fixes would make tests fail

[Sara Golemon]

On Fri, Feb 19, 2021 at 8:04 AM Christian Schneider 
wrote:

> Hi there,
> we encountered small annoyances over time which could be easy to fix but
> would break tests.
> Why does this matter? While it is easy to fix the PHP test suite they
> could potentially affect other projects' code, most probably tests.
>
> Here are the two things on our current list:
>
> Stack traces:
> ==
> Stack traces for certain constructs do not include the exact location in
> the stack frame:
> $a[(object)null] = 1;
> $e->getTrace() => []# <-- no stack trace
> while for
> file(); / file(null);
> $e->getTrace() => [ ["file"]=> .. ].
>
>
What version are you testing with. I know Niki killed some unnecessary
frames in 8.0's stacktrace output. (A whole repro script is more useful
than a snippet wherein $e comes into being seemingly from nowhere).



> Indentation in var_export:
> 
> The indentation for
> var_export((object)[(object)[]]);
> is off by one for nested structures:
> (object) array(
>'0' =>   # <-- extra space before index
>   (object) array(
> and the fix would be to change
> buffer_append_spaces(buf, level + 2)
> to
> buffer_append_spaces(buf, level + 1)
> in ext/standard/var.c
>
> This breaks about 60 PHP tests relying on the var_export format.
>
>
I think shifting the indentation in var_export() could probably be done
without a major BC break.  Trimming trailing whitespace would also resolve
one of my long-time annoyances with the function.
Feel free to propose that as a PR and/or an RFC.

-Sara