Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

isset()/empty() not working correctly with chained references #373

Open
viktorasp opened this issue Aug 1, 2024 · 0 comments
Open

isset()/empty() not working correctly with chained references #373

viktorasp opened this issue Aug 1, 2024 · 0 comments

Comments

@viktorasp
Copy link

If you are are poor soul stuck with an old code migration, please note that when migrating from PHP5 to PHP7+, the checks isset()/empty() on chained references might stop working correctly:

$user = User::Load($someId);
if (isset($user->Group->name)) {
    echo "name is set"; // PHP5
} else {
    echo "name is missing"; // PHP7+
}

After spending way too many hours on this issue, the problem is a combination of a tiny change in PHP and Doctrine1 dynamic properties usage.

PHP changes explanation:

Sandbox URL: https://onlinephp.io/c/89306

<?php
class Registry
{
    protected $_items = array();
    public function __set($key, $value)
    {
    	echo "Set: $key\n";
        $this->_items[$key] = $value;
    }
    public function __get($key)
    {
    	echo "Get: $key\n";
    	return isset($this->_items[$key]) ? $this->_items[$key] : null;
    }
    public function __isset($key)
    {
    	echo "Isset: $key\n";
        return isset($this->_items[$key]) ? empty($this->_items[$key])) === false : null;
    }
}
class RegB extends Registry {}

$registry = new Registry();
$registry->one = new RegB();
$registry->one->two = 'value';


var_dump(empty($registry->one->two)); 

Running the code above, there's a small difference: when doing check empty($registry->test->secret), PHP7+ does isset() check for each element in a chain, starting from the left side. So, it goes like this
PHP7+:
Isset: one
Get: one
Isset: two
Get: two

PHP5:
Get: one
Isset: two
Get: two

Now we are getting to the Doctrine1 part!

Doctrine/Record.php:1782 has

if (isset($this->_references[$offset]) &&
    $this->_references[$offset] !== self::$_null) {
    return true;
}

which checks if a given offset exists among references, and in PHP5 that works just fine, as we are checking the right-most value in the chain for existence. In PHP7+ this check fails, as we are checking the left-most chain member, and if it's not loaded yet, then this check will fail.

As for the possible fixes, either user-land code could change isset()/empty() to a bunch of validity and instanceof checks, in case of the original example, something like:

$user = User::Load($someId);
if ($user->Group instanceof Group && isset($user->Group->name)) {
// if ($user->Group && $user->Group->name) {
    echo "name is set"; // hooray!
} else {
    echo "name is missing";
}

optionally, Doctrine/Record::contains() could be modified to attempt loading of a reference, if it was not found immediately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant