PHP 7.1 - Address PHPSadness #28? View as Markdown

On Tue, Aug 25, 2015 at 11:11 PM, Sherif Ramadan
theanomaly.is@gmail.com wrote:

That's exactly what isset() does. isset/empty never raise errors.

On Tue, Aug 25, 2015 at 11:09 PM, Scott Arciszewski scott@paragonie.com
wrote:

Hi everybody,

Would anyone be interested in adding another helper like
isset()/empty() simply called exists() which would return true if the
variable is defined in the current scope (i.e. without raising an
E_NOTICE)?

It should be a simple change to add this function but it's too late
for 7.0 so, if there is any interest, I would respectfully put it off
until 7.1.

Scott Arciszewski
Chief Development Officer
Paragon Initiative Enterprises https://paragonie.com

--

http://phpsadness.com/sad/28
https://3v4l.org/2vrKG

Not quite.

Scott Arciszewski
Chief Development Officer
Paragon Initiative Enterprises <https://paragonie.com

Would anyone be interested in adding another helper like
isset()/empty() simply called exists() which would return true if the
variable is defined in the current scope (i.e. without raising an
E_NOTICE)?

Not sure what the use case would be for this. For most practical
purposes, isset/empty look completely fine.

As a web (not core) developer I've been bitten by this a couple of times.

Both were fairly quick to identify/fix, but enough to be a little annoying.

If you want some example of where I got this wrong...

I have a config array for defining the websites CSP header, it starts something like:

$csp_directives = array(
        'default-src'  => array("'none'"),
        'form-action'  => array("'self'"),
        'style-src'    => array("'self'"),
        'img-src'      => array("'self'"),
        'script-src'   => array("'self'"),
    );

Depending on the website I may then add a key to define the 'report-uri', or I may set it to NULL to say that I don't want one. If it remains unset (key does not exist), then the framework provides a default. At first I used an isset() check, forgetting that setting to NULL would not be seen as "set".

When creating a URL builder that allowed the query string to be passed in as an array. If it defaulted to the current URL (with its query string values), then the passed in array might tell it to explicitly remove certain values by setting them to NULL. When I first wrote this, I did an isset() check when adding the current URL's query string to the array of query string values (which take precedence).

Neither were major problems, just a slight annoyance (i.e. identified and fixed before I committed the code). But I'm normally using an isset() as the proposed exists() function would work.

Hi!

Would anyone be interested in adding another helper like
isset()/empty() simply called exists() which would return true if the
variable is defined in the current scope (i.e. without raising an
E_NOTICE)?

Not sure what the use case would be for this. For most practical
purposes, isset/empty look completely fine.

It should be a simple change to add this function but it's too late
for 7.0 so, if there is any interest, I would respectfully put it off
until 7.1.

It is simple, but not everything that is simple should be done.
Especially when we talking about adding core functions.

--
Stas Malyshev
smalyshev@gmail.com

Benoit Schildknecht wrote on 15/09/2015 18:04:

Every dev I've seen in my company use isset() as its name says it does
: "Is this variable set, whatever its value is ?". That's exactly how
we use it in the code.

Can you give an example of code where you do not know this until the
code runs - i.e. where "is this variable set?" is something you can hang
business logic on?

Somewhere where it would make sense to write something like this, if the
exists() function were available for plain variables:

if ( exists($a) ) {
...
} elseif ( is_null($a) ) {
...
} else {
...
}

The irony is, we set a variable to "null" mostly to avoid "Undefined
variable" notices. To explain it shortly, when we see "$var = null;",
we know that it is a variable that could not be changed in the scope.

Why is that ironic? That's exactly what the notice is telling you to do

• assign an explicit value to your variables to say "this variable
  intentionally left blank".

It is logical (at least for us) that "isset()" returns true when a
variable exists, even if its value is "null". I've yet to discover a
security bug in our code, but we have very sensitive applications, in
which we widely use "isset()". It could cause a lot of damages (maybe
it already has, but haven't noticed yet), and "exists()" would prevent
them.

Again, I challenge you - or anyone - to give a single, theoretical,
example of such a bug. The only ones I can think of are made worse by
use of exists(), not better, because they allow programmers to write
extremely brittle code, as in the example I posted earlier of
refactoring to extract code into a new function:

...
unset($foo);
refactored_code($foo);
...

function refactored_code($bar)
{
if exists($bar) // oops, this is always true
...
}

This is why I'm so opposed to the suggestion, because not only can
people use it to write really bad code, I've yet to see an example where
it would lead to good code. If we're interested in the reputation of the
language, adding facilities for doing things badly doesn't sound like a
good move.

Regards,

Rowan Collins
[IMSoP]

Stig Bakken wrote on 18/09/2015 14:44:

What lesson can we learn from that? Short and sweet names sometimes
lead to problems, if they do not precisely convey the actual
semantics.

I would much rather see a function called something like
is_variable_defined(), to avoid running into the same trap with
exists(). No room for assumptions that lead you to not check the docs.

I agree 100% with this. If we do add a new function, which I'm still
not entirely convinced of the need for, we should be very explicit about
what it does, and very careful in naming it.

Regards,

Rowan Collins
[IMSoP]

I want to pull this out for a bit more attention: one of the crucial questions in this thread is whether the language is wrong, or just the documentation. There's one particularly wonky passage someone found in the manual that makes it sound like unitialised variables have an intrinsic type when accessed, rather than just referring the reader to the rules on casting null to any given type; when I have time, I will find and reword it.

The docs suggest that uninitialized variables are null, and the above makes it sound like that’s what you’re stating, too. But they’re not: they don’t exist at all, they’re uninitialized. If they were null, then PHP wouldn’t spit out errors about undefined variable accesses because it would see them as the same thing as a (perfectly legal) null variable access. Differentiating these various cases is currently a little wonky, requiring some extra boilerplate code and/or ugly-ish workarounds (like calling get_defined_vars()). I don’t think it’s a huge gap in the language, but it’s a definite gap, and it’s one that many less-experienced programmers fall right into with safety checks.

-Bob

1 PHP defines null to include variables that have "not been set" 2

No, not really, PHP does not define that.

It does according to the docs:

"A variable is considered to be null if […] it has not been set to any value yet."

http://php.net/manual/en/language.types.null.php http://php.net/manual/en/language.types.null.php

I think that’s more sloppy documentation (declared versus defined) as opposed to literal truth, but….

PHP also defines null to include variables that have been unset()

No, PHP does not define anything like that. In fact, unset() means the
variable is destroyed and does not exist (of course, excepting refcount
etc. issues). Null does not feature in this story at all.

I’ll refer again to the above documentation:

"A variable is considered to be null if […] it has been unset().”

This one can’t be chalked up to sloppy writing. Here, something is just plain wrong.

All of the above does touch on the gap, though. Ignoring the docs and looking at actual behavior of PHP, it’s clear that an undefined variable behaves differently from one assigned null. One can easily tell if a variable is null, but one can’t easily tell if a variable is just not defined. That strikes me as an oversight especially given the fact that one can use unset() to make a variable once again undefined. Put another way, we can create and destroy, but we can’t identify existence. It’s a small gap in language design. There are workarounds, but they’re workarounds.

That's not a correct description. Correct description is that if you ask
for a variable and it is not defined, null value is substituted.

That happens in the end, but not before an error is thrown. I want a one-keyword way to know that a variable is not defined without having to trip an error.

That's your opinion. Mine, for example, is that it is a very useful
feature. In any case, PHP is implemented this way, and there's no way to
change it while the result remaining PHP language. If it makes PHP
unsuitable for you, sorry, but that's what PHP is. I don't think it
makes a lot of sense to discuss changing basic semantics of PHP engine -
I don't think this is going to happen.

At this point, I don’t think anyone is proposing changing the semantics of the engine, just filling a gap that lets one determine whether a variable has already been defined -- like one can already do for constants, functions, classes, array elements, and object properties. You may or may not have use for the functionality, but other people do, and adding a clean way to determine how PHP would regard a given variable doesn’t strike me as in any way promoting bad code even if you don’t personally use it.

-Bob

Lester Caine wrote on 16/09/2015 11:35:

If you are working with an array then fine, but we are talking about
variables here not arrays. isset($i) is a local variable so 'which array
is the variable is hiding in' applies?

No, it's not hiding in any array, and I agree that array_key_exists() is
not the right function to use.

However, I have still yet to see any use case where it would be
necessary or useful to detect - at runtime, rather than in static
analysis for debugging - that $i has never been initialised, vs it
having been initialised to null.

Regards,

Rowan Collins
[IMSoP]

If the extract() was being used to make the view code more readable in
the first place, the exists() defeats rather than assists in that
objective, in my opinion.

My own interface with it has been via templating, where the layout is
stored in a database and as elements are download any 'NULL' values
either get replaced by a default, or ignored. It's not the best of
coding practice, but it works ...

--
Lester Caine - G8HFL

Contact - http://lsces.co.uk/wiki/?page=contact
L.S.Caine Electronic Services - http://lsces.co.uk
EnquirySolve - http://enquirysolve.com/
Model Engineers Digital Workshop - http://medw.co.uk
Rainbow Digital Media - http://rainbowdigitalmedia.co.uk

I can certainly sympathise with wanting to run without any notices - they are generally hints for writing better code. In the vast majority of cases, though, that means working out why the variable is undefined, otherwise you might as well just use the @ operator to say "I don't care about this notice”.

Agree completely, and for that reason, we also ban @ except in those rare cases where PHP forces the issue (that’s for other discussions...). Indeed, this often comes up with short-lived issues that are resolved quickly. For example, a config file is missing a variable, so the user is told what it is, what kind of value to supply, and they goes off to fix it, or the user is warned and the app goes on with a default. But, all this happens without throwing errors and without having to detour through the global error handler. In other cases, the fix may not be so available, like with the template system that creates variables in the view’s scope; here, the goal is just to gracefully catch the condition and then define a default (and maybe complain to the author).

This still implies that there's a common problem to fix, which I just don't believe. The number of places where the following all hold true must be vanishingly small:

• the third-party code is dumping bare variables into your scope (globals, or a file-level include are the only mechanisms I can think of)

This seems to be pretty common with template systems, especially older ones. It’s also common with config files.

• they do so unreliably, in the sense that different variables will be set under different circumstances

More common than different variables in different circumstances is the missing variable. Consider the user who’s setting up an instance of an application that uses a config file with bare variables — very easy to forget something, or type the name wrong, or whatever. Or the programmer writing a plugin for a poorly documented application who wants to be sure the runtime environment of the plugin is what’s expected. Really, a lot of the same situations where one might need to check the existence of an array element or object property, except that the item to be checked happens to be a plain old variable.

• you/they have used null as a value which needs to be positively asserted, not taken as the default state, so that isset() does not meet your needs

Well, it’s not so important that null represents a non-default state, as that it represents any state. Often, null could just signal that a default should be used, but with an existence check, the programmer can be confidant that something/someone explicitly chose that option. In the case of a config, for example, you can be sure the user chose a particular value (which may be the default) versus having missed it altogether. There are cases where that knowledge is very important to safe programming.

Consider that you’ve refactored a delete function to add support for non-permanent deletion. You’ve done this by adding an optional parameter:

function DeleteFile($file, $permanentlyDelete = null) {}

If $permanentlyDelete is null, the default action specified in a config is used. The problem is, when the param is null, there’s no way for the function to know for sure what the programmer of the caller intended. There are two possibilities:

1 The programmer intentionally passed null because the config should be used
2 The programmer forgot to consider that case after the refactor, so null is automatically being used

Ultimately, the function just follows orders, of course, but doing it this way is dangerous because it’s all too easy to hit situation 2, which means files could be permanently deleted in error. As the programmer doing the refactor, it’s better to make the parameter mandatory, which forces a revisit to every caller to make the true/false/null decision explicit.

In this case, the extra bit of intelligence comes from forcing the caller to pass something. If the param is optional, we have no idea. It’s the same thing with exists() on a variable: it gives us extra intelligence about why a variable is null, whether it’s because of an actual decision somewhere or because of an error or oversight. And it provides that intelligence without fussing with PHP errors.

• you have no way of presetting the variable to some other terminal value to detect when it is explicitly set

If the variables are injected before your code runs, it’s too late. At that point, if you try to set a magic sentinel value, you’re just overwriting the real value if there is one. It would work, however, if you can pre-initialize before the injection:

$foo = 'hope this gets overwritten';
require('/some/config.php');
if ($foo === 'hope this gets overwritten') {
//uh-oh
}

I’m not really a fan of this pattern, though. If there’s going to be a magic value to indicate that no better value was set, null is the ideal choice:

$foo = null;
require('/some/config.php');
if ($foo === null) {
//uh-oh
}

Here, we’ve used null exactly as intended. Unfortunately, this breaks if null already has meaning, even if it's just "go with a default”. And, of course, it also throws the annoying undefined error that we’re trying to avoid :-).

This is where exists() comes in and cures all:

require('/some/config.php');
if (!exists($foo)) {
//uh-oh
}

It’s cleaner and shorter, but it also lets us again use null as intended, rather than coming up with some whacky magic value that by necessity is completely unrelated to the variable’s purpose yet is hopefully understandable in meaning to future programmers passing through (not too hard with a string, as above, but much tricker with non-strings).

In a dynamic
language like PHP, I really don’t understand how folks can not see
that as an oversight.

I've not found evidence that any other language treats variables in the proposed way, so it really doesn't feel like an "oversight" to me, but a request for a new and quite unusual feature.

In JavaScript, there’s this:

if (typeof never_heard_of_me === 'undefined') {
//uh-oh
}

albeit with a couple of minor caveats.

But yes, from a completeness point of view, it does make some sense. My fear is that the people who don't understand why isset() does what it does will think it's the answer to some imaginary problem, and end up more confused than ever when their code breaks.

Haven’t we already arrived here with isset() confusion? ;-)

Seriously, I think updating the docs on the isset() page would help mitigate this. From this:

"Determine if a variable is set and is not NULL.

"If a variable has been unset with unset(), it will no longer be set. isset() will return FALSE if testing a variable that has been set to NULL. Also note that a null character ("\0") is not equivalent to the PHP NULL constant."

to something like this:

"Determine if a variable exists in the current scope and is not NULL. To test only whether a variable exists, use exists().

"If unset() is called on a variable, it will no longer exist. isset() will return FALSE if testing a variable that has been set to NULL. Also note that a null character ("\0") is not equivalent to the PHP NULL constant."

Obviously, other related pages could be updated similarly.

If the function had been proposed as a debugging aid, maybe filed under Reflection or debug_variable_is_initialised, I might have more sympathy, but that's not how it's been presented.

Again, I don’t see this as something that directly controls business logic. I see it as something that supports defensive programming, improved error handling, and improved user experience. Regardless, as we all know, every language feature can be abused, but that doesn’t automatically mean we shouldn’t have those features. Even something as widely derided as goto has its niche, after all (albeit, a very, very small niche). In this case, not only is the feature useful, but it nicely complements the array of related functionality to provide a more complete feature set.

-Bob

'exists but I don't know what

as yet' is a logical state
It is indeed. "I know it's name but it doesn't exist", however, is
generally not a valid state - if you've hard-coded the name (as is the
case with 99.9% of bare variables) then clearly you consider it to have
some existence. For this reason, PHP assumes that you meant to create
it with a null value.

If I have a dozen objects 'hard coded', but the templating system only
needs to use a few of them for this page then only the variables that
have been created exist. Yes one would be better off scanning an
associative array and only dealing with the elements actually present
in
the array. That is better coding practice, but extract() is an equally
valid practice currently being used and that needs a different way of
handling if the elements exist.

I'm still not entirely sure what it is you need to "handle".

What is the behaviour that you'd put in the if(! exists ()) block? More particularly, what would you put there and not put in the if(! is_null()) block, since if they're the same, then that's what if(! isset()) does, so we're all good?

Hi,

On 17 September 2015 at 19:01, Rowan Collins rowan.collins@gmail.com
wrote:

On 17 September 2015 02:17:59 BST, Robert Williams rewilliams@thesba.com
wrote:

An unset variable is not null. Rather, it’s
completely undefined, and PHP yells at you for just that reason if you
try to access it. Then, it turns around and tries to appease you by
giving you null.

I see it as rather the other way around: it happily gives you a null, but
then says "oh, by the way, you never actually initialised that variable to
null". Not an error, just a friendly aside.

--

Here is an example where exists would be useful;
// input is {name:'Ted', birth_dt: null}
$input = json_decode(file_get_contents('php://input'));
// I should have used property_exists() instead of isset().. wasn't
expecting a null..
if ( isset($input->birth_dt) && ! isValidBirthDate($input->birth_dt) )
{
throw new Exception('Bad birth date');
}
saveToDb($input); // Oh no... I clobbered birth date

​Terry​

Lester Caine wrote on 23/09/2015 18:15:

I'm still in favor of having a variable_exists('varname') function though.
We had it ...
https://bugs.php.net/bug.php?id=24274

Actually that looks like what is currently being proposed as exists() -
a swiss-army knife function based on isset() which is called as though
you're examining the value.

Instead of this:

variable_exists($foo): yes (should be yes)
unset($foo);
variable_exists($foo): no (should be no)
variable_exists($bar): no (should be no)

My suggestion (with which I believe Stig was agreeing) is this:

variable_exists('foo'): yes (should be yes)
unset($foo);
variable_exists('foo'): no (should be no)
variable_exists('bar'): no (should be no)

François Laupretre wrote on 23/09/2015 22:17:

Le 23/09/2015 18:37, Stig Bakken a écrit :

If they start replacing calls to
isset()/array_key_exists() with exists(), that's their choice and it
can
only improve overall readability.

The thing is, it won't really improve overall readability, because the
name exists() is still too vague. It may be obvious to the person who
wrote the code (at least for the first two weeks), then you're really
back into the same vagueness that isset() provides.

I don't say it's more readable than isset(), I say that's more
readable than array_key_exists().

To be pedantic, you actually did say that, in the text quoted above:
"replacing calls to isset() ... with exists() ... can only improve
overall readability".

Regardless, Stig's point stands, that exists() is a very general name,
and the proposed function definition is also very all-inclusive, leading
to the risk of hard-to-understand code.

Regards,

Rowan Collins
[IMSoP]