Re: [PHP-DEV] Setting to disable the "Undefined array index" warning
On Tue, Feb 15, 2022, at 6:54 AM, Andreas Leathley wrote: > On 15.02.22 13:31, Nicolas BADIA wrote: >> As it is explained in this ticket https://bugs.php.net/bug.php?id=81417 we >> use to check if a property exists by accessing it directly like we do in >> JavaScript. >> >> Personally, I subscribe to this coding style and we use it all over our >> codebase (more than 130 000 lines of PHP code). When it became a notice, we >> disabled them, but now that it is a warning, it is a lot more problematic >> for us… We can’t even use the last version of PHP Debug for VSCode. > > The problem with your way of writing code is that it is ambiguous in > meaning, which is why this is a warning. You are not checking if a key > in an array exists, you are checking if an array value is "false-y". If > the value is an empty string, it is also interpreted as false, if it is > null, it is also interpreted as false, if it is an integer of value zero > it is also interpreted as false, if it is the string "0" it is also > interpreted as false. > > When you write code as you do, it is easy to introduce subtle errors > because of these implicit casts. If you want to check if an array key is > defined, you should do it explicitly - this is not about a "coding > style", this is about writing the code in a way that actually does what > you intend it to do without it meaning multiple things of which some are > probably not expected. If you just want a quick fix without solving the > underlying problem, you can just add "?? null" to each array key access. > > If you want to upgrade/improve your code base, which is a worthwhile > task, I would suggest the use of a static analyzer (like Psalm or > PHPStan) to find out where your code is ambigious. I have found many > undetected errors that way, where implicit casts have lead to unexpected > outcomes, which is why I am very much in favor of these warnings for > undefined array keys. As a data point, TYPO3 is a roughly 500,000 line code base. (Non-comment lines of code as reported by phploc.) It also relied very heavily on the old "silently ignore missing values and pretend it's a null" behavior, which broke badly in PHP 8. It took one person (me) about 3 weeks I think of running unit tests, adding ?? in various places (or array_key_exists, or whatever made sense in context), running tests again, etc. to fix nearly all of them. There's still a few that pop up now and again that didn't have test coverage, but they're rare. And that's me doing it all 100% manually, no Rector or similar automation tools. Yes, there is work required for this change. However, with a code base 1/4 the size, and using better automation tools than I did you should be able to address all the upgrade issues in less than one person-week. And that's without even getting into the question of array-centric code with properties being maybe-undefined is already a code smell, and has been since PHP 4. (There's been a lot of very smelly code from the PHP 4 era, but it was smelly even then.) Even without any (probably good) changes to the architecture or business logic of the application, this would improve the quality of the application. I'd wager it's less work in terms of raw time to just fix up the code base than it is to write, implement, debate, pass, and merge an RFC to make you not have to do so. --Larry Garfield -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: https://www.php.net/unsub.php
Re: [PHP-DEV] Setting to disable the "Undefined array index" warning
On 15/02/2022 15:12, Guilliam Xavier wrote:
On Tue, Feb 15, 2022 at 3:07 PM Rowan Tommins
wrote:
if ( array_key_exists('key', $array) && $array['key'] !== null &&
$array['key'] !== false && $array['key'] !== 0 && $array['key'] !== 0.0
&& $array['key'] !== '' && $array['key'] !== [] )
Agreed, but you forgot the most "annoying":
&& $array['key'] !== '0'
Ah yes, that one really demonstrates the dangers of type juggling in
general. If it _wasn't_ considered empty, that would be inconsistent
with how integer juggling works, but it definitely catches people out!
Mind operator precedence! This is interpreted as:
if ( $array['key'] ?? (false === true) )
(obviously not intended), so you need explicit parentheses:
if ( ($array['key'] ?? false) === true )
Oops, I admit I didn't test that. That does make the operator somewhat
less attractive, and maybe it would be more straight-forward to just be
explicit:
if ( isset($array['key']) && $array['key'] === true )
I will also note that the "shorthands" (!empty(), isset(), ?? null) will
also "hide" another warning when the $array variable itself is undefined
Ah, yes, so they do; as does the ?? operator, for that matter.
I seem to remember someone proposing an explicit operator for "this
array dimension might not exist", something like this:
if ( $array[?'key'] === true )
Which would translate to:
if ( (array_key_exists('key', $array) ? $array['key'] : null) === true )
Regards,
--
Rowan Tommins
[IMSoP]
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php
Re: [PHP-DEV] Setting to disable the "Undefined array index" warning
On Tue, Feb 15, 2022 at 3:07 PM Rowan Tommins
wrote:
> On 15/02/2022 12:54, Andreas Leathley wrote:
> > The problem with your way of writing code is that it is ambiguous in
> > meaning, which is why this is a warning.
>
>
> I think that's a good way of looking at it. There's actually quite a lot
> of code hiding a check like "if ($array['key'])"; roughly speaking, it's
> short-hand for:
>
> if ( array_key_exists('key', $array) && $array['key'] !== null &&
> $array['key'] !== false && $array['key'] !== 0 && $array['key'] !== 0.0
> && $array['key'] !== '' && $array['key'] !== [] )
>
Agreed, but you forgot the most "annoying":
&& $array['key'] !== '0'
> Now, if that's what you intended, there's a syntax that's slightly more
> explicit while still being reasonably short: the empty() pseudo-function:
>
> if ( ! empty($array['key']) )
>
> On the other hand, if what you actually meant was this:
>
> if ( array_key_exists('key', $array) && $array['key'] !== null )
>
> Then you might have some bugs lurking, and actually want the isset()
> pseudo-function instead:
>
> if ( isset($array['key']) )
>
> For other cases, you do have to spend a few more characters to be
> explicit, often using the "??" operator; for instance, if you expect
> $array['key'] to be either unset or a boolean, and want this:
>
> if ( array_key_exists('key', $array) && $array['key'] === true )
>
> Then the shortest is probably something like this:
>
> if ( $array['key'] ?? false === true )
>
Mind operator precedence! This is interpreted as:
if ( $array['key'] ?? (false === true) )
(obviously not intended), so you need explicit parentheses:
if ( ($array['key'] ?? false) === true )
I will also note that the "shorthands" (!empty(), isset(), ?? null) will
also "hide" another warning when the $array variable itself is undefined
Re: [PHP-DEV] Setting to disable the "Undefined array index" warning
On 15/02/2022 12:54, Andreas Leathley wrote:
The problem with your way of writing code is that it is ambiguous in
meaning, which is why this is a warning.
I think that's a good way of looking at it. There's actually quite a lot
of code hiding a check like "if ($array['key'])"; roughly speaking, it's
short-hand for:
if ( array_key_exists('key', $array) && $array['key'] !== null &&
$array['key'] !== false && $array['key'] !== 0 && $array['key'] !== 0.0
&& $array['key'] !== '' && $array['key'] !== [] )
Now, if that's what you intended, there's a syntax that's slightly more
explicit while still being reasonably short: the empty() pseudo-function:
if ( ! empty($array['key']) )
On the other hand, if what you actually meant was this:
if ( array_key_exists('key', $array) && $array['key'] !== null )
Then you might have some bugs lurking, and actually want the isset()
pseudo-function instead:
if ( isset($array['key']) )
For other cases, you do have to spend a few more characters to be
explicit, often using the "??" operator; for instance, if you expect
$array['key'] to be either unset or a boolean, and want this:
if ( array_key_exists('key', $array) && $array['key'] === true )
Then the shortest is probably something like this:
if ( $array['key'] ?? false === true )
Regards,
--
Rowan Tommins
[IMSoP]
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php
Re: [PHP-DEV] [RFC] [Under Discussion] Random Extension 4.0
Hi
On 2/15/22 12:48, Go Kudo wrote:
At first, I updated the RFC to the latest status.
https://wiki.php.net/rfc/rng_extension
Thank you, the examples are useful.
I need some time to think about the current issue. I understand its
usefulness, but I feel uncomfortable with the fact that the NumberGenerator
generates a string.
Would you feel more comfortable if the interface would be called
\Random\Engine or \Random\Generator (i.e. leaving out the "Number" from
the interface name)?
Engine is the term used by C++:
https://en.cppreference.com/w/cpp/numeric/random/mersenne_twister_engine
Generator is the term used by Java:
https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/random/RandomGenerator.html
--
With the 'FixedForUnitTest' example you mentioned in the RFC: While for
that specific implementation it appears pretty obvious that increasing
numbers are generated, in practice:
1. This will result in inconsistent behavior based on the architecture.
I can't test it due to the lack of the necessary architectures, but I
believe the following to be accurate:
$g = new FixedForUnitTest();
$r = new Randomizer($g);
// 0100 in 64 Bit little endian
// 01000200 in 32 Bit little endian
// 0001 in 64 Bit big endian
var_dump(bin2hex($r->getBytes(8)));
2. This analogy completely breaks down for the 'shuffle' functions which
call the generator internally an unspecified number of times:
$g = new FixedForUnitTest();
$r = new Randomizer($g);
var_dump($r->shuffleString("abcdefghijklmnopqrstuvwxyz")); //
wosqyrupatvxznmlkjihgfedcb
var_dump($r->shuffleString("abcdefghijklmnopqrstuvwxyz")); //
fwrtjndlsyvpzuhxbqomkigeca
The resulting strings look somewhat ordered, but there is no clear
pattern, despite the underlying generator being completely predictable!
I also wonder about the point of changing RNG to XorShift128Plus. There are
a number of derived implementations, which RNG do you think is more
suitable?
I'm not an expert in RNGs, but based off this page:
https://prng.di.unimi.it/ and the linked papers it appears that
xoshiro256** is the overall best choice if memory usage is not a concern.
Best regards
Tim Düsterhus
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php
Re: [PHP-DEV] Setting to disable the "Undefined array index" warning
Le 15/02/2022 à 13:46, Thomas Nunninger a écrit :
Hi Nicolas,
as far as I understand, adding new ini settings is not the way to go
as this would mean that your code behaves differently on different
environments.
Two suggestions:
* Write your own error handler that ignores those errors.
* Try some tool like Rector to rewrite your code.
Best regards
Thomas
Hello,
I do agree with Thomas here, changing PHP internal behavior by
configuration is a dangerous thing to do.
If I understand correctly, there is "more than 130 000 lines of PHP
code" to fix, believe me I migrated a few projects along the way from
PHP 5.6 to 8.0 over time (one was about this size, but it stopped at PHP
7.0 a few years back). They probably contain less "legacy code" since we
have strict conventions from the beginning, but we did migrate some
projects still.
Fact is, a legacy project cannot run without evolving if you want to
support a recent PHP version. The language itself evolves, and that's
for the best. Now for really legacy unmaintained projects, there's still
the solution to keep them running on deprecated PHP version using some
container technology. It's not ideal, but it can help until you finally
find the necessary resources to port.
Using a good static analysis tool and possibly some other tools such as
Rector, 130k lines is not an impossible goal.
I don't think there's an easy solution here, it's either don't upgrade,
or either refactor, but modern tooling should help you a lot with that.
If it can help you, can can still replace your array access using `$a =
$foo['bar'] ?? null` instead of `$a = \array_key_exists('bar', $foo) ?
$foo['bar'] : null`, it's both easier to read and write (you just store
` ?? null` in your clip board and hit paste on every use case you find).
Hope it helps,
Regards,
--
Pierre
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php
Re: [PHP-DEV] Setting to disable the "Undefined array index" warning
On 15.02.22 13:31, Nicolas BADIA wrote: As it is explained in this ticket https://bugs.php.net/bug.php?id=81417 we use to check if a property exists by accessing it directly like we do in JavaScript. Personally, I subscribe to this coding style and we use it all over our codebase (more than 130 000 lines of PHP code). When it became a notice, we disabled them, but now that it is a warning, it is a lot more problematic for us… We can’t even use the last version of PHP Debug for VSCode. The problem with your way of writing code is that it is ambiguous in meaning, which is why this is a warning. You are not checking if a key in an array exists, you are checking if an array value is "false-y". If the value is an empty string, it is also interpreted as false, if it is null, it is also interpreted as false, if it is an integer of value zero it is also interpreted as false, if it is the string "0" it is also interpreted as false. When you write code as you do, it is easy to introduce subtle errors because of these implicit casts. If you want to check if an array key is defined, you should do it explicitly - this is not about a "coding style", this is about writing the code in a way that actually does what you intend it to do without it meaning multiple things of which some are probably not expected. If you just want a quick fix without solving the underlying problem, you can just add "?? null" to each array key access. If you want to upgrade/improve your code base, which is a worthwhile task, I would suggest the use of a static analyzer (like Psalm or PHPStan) to find out where your code is ambigious. I have found many undetected errors that way, where implicit casts have lead to unexpected outcomes, which is why I am very much in favor of these warnings for undefined array keys. -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: https://www.php.net/unsub.php
Re: [PHP-DEV] Setting to disable the "Undefined array index" warning
Hi Nicolas, as far as I understand, adding new ini settings is not the way to go as this would mean that your code behaves differently on different environments. Two suggestions: * Write your own error handler that ignores those errors. * Try some tool like Rector to rewrite your code. Best regards Thomas Am 15.02.22 um 13:31 schrieb Nicolas BADIA: Hi, As it is explained in this ticket https://bugs.php.net/bug.php?id=81417 we use to check if a property exists by accessing it directly like we do in JavaScript. Personally, I subscribe to this coding style and we use it all over our codebase (more than 130 000 lines of PHP code). When it became a notice, we disabled them, but now that it is a warning, it is a lot more problematic for us… We can’t even use the last version of PHP Debug for VSCode. As I’m not ready to spend weeks upgrading our codebase because our coding style is not supported by the community, I’m looking at alternatives. The more obvious and simple for me would be to add a setting in php.ini to allow this. So I’d like to know if it is something you would support. If not, I guess I will have to fork PHP, but this will complicate a lot our deployment process and make our code not portable. As I believe we are not the same in this case, I hope you will support the addition of a setting… Thanks in advance for considering this. Best regards, Nicolas -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: https://www.php.net/unsub.php
Re: [PHP-DEV] [RFC] [Under Discussion] Random Extension 4.0
2022年2月15日(火) 19:03 Tim Düsterhus :
> Hi
>
> On 2/15/22 04:58, Go Kudo wrote:
> >> Regarding "unintuitive": I disagree. I find it unintuitive that there
> are
> > some RNG sequences that I can't access when providing a seed.
> >
> > This is also the case for RNG implementations in many other languages.
> For
> > example, Java also uses long (64-bit) as the seed value of the argument
> for
> > Math.
> >
> >
> https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/Random.html#%3Cinit%3E(long)
>
> java.util.Random is a LCG with only 48 Bits of state. A single 64-bit
> signed long is sufficient to represent the state.
>
> > On the other hand, some languages have access to the complete internal
> > state. Python, for example, accepts bytes or bytearrays.
> >
> > https://docs.python.org/3/library/random.html#random.seed
> >
> > However, making strings available in PHP may lead to incorrect usage.
> >
> > I think we can safely do this by making the seed argument accept both int
> > and string, and only using it as the internal state if string is
> specified
> > and it's 128-bits long.
>
> That's a solution that would work for me.
>
> >> 1. Would you expect those two 'var_dump' calls to result in the same
> > output?
> >
> > Added __debugInfo() magic method supports.
> >
> >
> https://github.com/php/php-src/pull/8094/commits/78efd2bd1e0ac5db48c272b364a615a5611e8caa
>
> Don't forget to update the RFC accordingly. It would probably be helpful
> if you would put the full class stubs into the RFC. I find that easier
> to understand than a list of methods.
>
> >> generate() should return raw bytes instead of a number (as I suggested
> > before).
> >
> > I don't think this is a very good idea.
> >
> > The RNG is a random number generator and should probably not be
> generating
> > strings.
>
> I'd say that the 'number' part in RNG is not technically accurate. All
> RNGs are effectively generators for a random sequence of bits. The
> number part is just an interpretation of those random sequence of bits
> (e.g. 64 of them).
>
> > Of course, I am aware that strings represent binary sequences in PHP.
> > However, this is not user-friendly.
> >
> > The generation of a binary string is a barrier when trying to implement
> > some kind of operation using numeric computation.
>
> I believe the average user of the RNG API would use the Randomizer
> class, instead of the raw generators, thus they would not come in
> contact with the raw bytes coming from the generator.
>
> However by getting PHP integers out of the generator it is much harder
> for me to process the raw bits and bytes, if that's something I need for
> my use case.
>
> As an example if I want to implement the following in userland. Then
> with getting raw bytes:
> - For Randomizer::getBytes() I can just concatenate the raw bytes.
> - For a random uint16BE I can grab 2 bytes and call unpack('n', $bytes)
>
> If I get random 64 Bit integers then:
> - For Randomizer::getBytes() I need to use pack and I'm not even sure,
> whether I need to use 'q', 'Q', 'J', 'P' to receive an unbiased result.
> - For uint16BE I can use "& 0x", but would waste 48 Bits, unless I
> also perform bit shifting to access the other bytes. But then there's
> also the same signedness issue.
>
> Interpreting numbers as bytes and vice versa in C / C++ is very easy.
> However in PHP userland I believe the bytes -> numbers direction is
> easy-ish. The numbers -> bytes direction is full of edge cases.
>
> > If you want to deal with the problem of generated size, it would be more
> > appropriate to define a method such as getGenerateSize() in the
> interface.
> > Even in this case, generation widths greater than PHP_INT_SIZE cannot be
> > supported, but generation widths greater than 64-bit are not very useful
> in
> > the first place.
> >
> >> The 'Randomizer' object should buffer unused bytes internally and only
> > call generate() if the internal buffer is drained.
> >
> > Likewise, I think this is not a good idea. Buffering reintroduces the
> > problem of complex state management, which has been made so easy. The
> user
> > will always have to worry about the buffering size of the Randomizer.
>
> Unfortunately you did not answer the primary question. The ones you
> answered were just follow-up conclusions from the answer I would give:
>
> var_dump(\bin2hex($r1->getBytes(8)));
> var_dump(\bin2hex($r2->getBytes(4)) . \bin2hex($r2->getBytes(4)));
>
> As a user: Would you expect those two 'var_dump' calls to result in the
> same output?
>
> >> Why xorshift instead of xoshiro / xoroshiro?
> >
> > The XorShift128Plus algorithm is still in use in major browsers and is
> dead
> > in a good way.
>
> I believe that that the underlying RNG in web browsers is considered an
> implementation detail, no?
>
> For PHP this would be part of the API surface and would need to be
> maintained indefinitely. Certainly it would make sense to use the latest
> and greatest RNG, instead of
[PHP-DEV] Re: RFC request
On 15.02.2022 at 12:06, Nicolas BADIA wrote: > Hi, > It looks like I need to request RFC karma for my username nicolasbadia > I’d like to start an internal discussion about this: > https://bugs.php.net/bug.php?id=81417 > We have big troubles with the warning 'Undefined array index’ and we’d like > to have a php.ini setting to disable it. > Best regards, > Nicolas RFC karma granted. Best of luck! -- Christoph M. Becker -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: https://www.php.net/unsub.php
Re: [PHP-DEV] [RFC] [Under Discussion] Random Extension 4.0
Hi
On 2/15/22 04:58, Go Kudo wrote:
Regarding "unintuitive": I disagree. I find it unintuitive that there are
some RNG sequences that I can't access when providing a seed.
This is also the case for RNG implementations in many other languages. For
example, Java also uses long (64-bit) as the seed value of the argument for
Math.
https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/Random.html#%3Cinit%3E(long)
java.util.Random is a LCG with only 48 Bits of state. A single 64-bit
signed long is sufficient to represent the state.
On the other hand, some languages have access to the complete internal
state. Python, for example, accepts bytes or bytearrays.
https://docs.python.org/3/library/random.html#random.seed
However, making strings available in PHP may lead to incorrect usage.
I think we can safely do this by making the seed argument accept both int
and string, and only using it as the internal state if string is specified
and it's 128-bits long.
That's a solution that would work for me.
1. Would you expect those two 'var_dump' calls to result in the same
output?
Added __debugInfo() magic method supports.
https://github.com/php/php-src/pull/8094/commits/78efd2bd1e0ac5db48c272b364a615a5611e8caa
Don't forget to update the RFC accordingly. It would probably be helpful
if you would put the full class stubs into the RFC. I find that easier
to understand than a list of methods.
generate() should return raw bytes instead of a number (as I suggested
before).
I don't think this is a very good idea.
The RNG is a random number generator and should probably not be generating
strings.
I'd say that the 'number' part in RNG is not technically accurate. All
RNGs are effectively generators for a random sequence of bits. The
number part is just an interpretation of those random sequence of bits
(e.g. 64 of them).
Of course, I am aware that strings represent binary sequences in PHP.
However, this is not user-friendly.
The generation of a binary string is a barrier when trying to implement
some kind of operation using numeric computation.
I believe the average user of the RNG API would use the Randomizer
class, instead of the raw generators, thus they would not come in
contact with the raw bytes coming from the generator.
However by getting PHP integers out of the generator it is much harder
for me to process the raw bits and bytes, if that's something I need for
my use case.
As an example if I want to implement the following in userland. Then
with getting raw bytes:
- For Randomizer::getBytes() I can just concatenate the raw bytes.
- For a random uint16BE I can grab 2 bytes and call unpack('n', $bytes)
If I get random 64 Bit integers then:
- For Randomizer::getBytes() I need to use pack and I'm not even sure,
whether I need to use 'q', 'Q', 'J', 'P' to receive an unbiased result.
- For uint16BE I can use "& 0x", but would waste 48 Bits, unless I
also perform bit shifting to access the other bytes. But then there's
also the same signedness issue.
Interpreting numbers as bytes and vice versa in C / C++ is very easy.
However in PHP userland I believe the bytes -> numbers direction is
easy-ish. The numbers -> bytes direction is full of edge cases.
If you want to deal with the problem of generated size, it would be more
appropriate to define a method such as getGenerateSize() in the interface.
Even in this case, generation widths greater than PHP_INT_SIZE cannot be
supported, but generation widths greater than 64-bit are not very useful in
the first place.
The 'Randomizer' object should buffer unused bytes internally and only
call generate() if the internal buffer is drained.
Likewise, I think this is not a good idea. Buffering reintroduces the
problem of complex state management, which has been made so easy. The user
will always have to worry about the buffering size of the Randomizer.
Unfortunately you did not answer the primary question. The ones you
answered were just follow-up conclusions from the answer I would give:
var_dump(\bin2hex($r1->getBytes(8)));
var_dump(\bin2hex($r2->getBytes(4)) . \bin2hex($r2->getBytes(4)));
As a user: Would you expect those two 'var_dump' calls to result in the
same output?
Why xorshift instead of xoshiro / xoroshiro?
The XorShift128Plus algorithm is still in use in major browsers and is dead
in a good way.
I believe that that the underlying RNG in web browsers is considered an
implementation detail, no?
For PHP this would be part of the API surface and would need to be
maintained indefinitely. Certainly it would make sense to use the latest
and greatest RNG, instead of something that is outdated when its first
shipped, no?
Also, in our local testing, SplitMix64 + XorShift128Plus performed well in
terms of performance and random number quality, so I don't think it is
necessary to choose a different algorithm.
If this RFC passes, it will be easier to add algorithms in the
