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

Phalcon\Mvc\Model::__isset doesn't account for the PHP empty failover #13518

Closed
CameronHall opened this issue Oct 10, 2018 · 6 comments
Closed
Assignees
Labels
enhancement Enhancement to the framework

Comments

@CameronHall
Copy link
Contributor

CameronHall commented Oct 10, 2018

Expected and Actual Behavior

When checking empty on a protected model property (out of scope) you would expect to get a definitive result by invoking the getter (if it exists, if not trigger a warning??). Instead, it'll always failover to the Phalcon\Mvc\Model::__isset which checks if a relationship exists.

class MyModel extends \Phalcon\Mvc\Model
{
    protected $secretValue;

    public function setSecretValue($value)
    {
        $this->secretValue = $value;
    }

    public function getSecretValue()
    {
        return $this->secretValue;
    }
}

$model = new MyModel();
$model->secretValue = 123;
var_dump(empty($model->secretValue)); // false

Details

  • Phalcon version: 3.4.1
  • PHP Version: (php -v)
  • Operating System: Windows
  • Installation type: DLL Download
  • Server: Apache
    Other related info (Database, table schema):
@JABirchall
Copy link

This is expected behavour by the PHP engine. The same as __set() and __get() allows you to access private/protected properties of a class if designed that way.

@CameronHall
Copy link
Contributor Author

I know it says it on the documentation. What I'm saying is the expected behavior would be that __isset would handle this functionality. However, it looks like it was designed to only check if relationships exist. I believe it should be updated to check the getters of protected properties as well.

@JABirchall
Copy link

JABirchall commented Oct 10, 2018

I get where you are coming from. It seems that they already have this functionallity in place in the __set

if !manager->isVisibleModelProperty(this, property) {
    throw new Exception("Property '" . property . "' does not have a setter.");
}

But do not use it in __get or __isset. Prolly have to wait for niden or klay to respond about that decision.

@niden
Copy link
Member

niden commented Oct 10, 2018

I think it is a good addition to the framework (both for __get and __isset)

However I don't want this in 3.4.x series since it could very well have unexpected results to existing applications. We should get this in 4.x

@phalcon/core-team Thoughts?

@JABirchall
Copy link

@niden I would also consider changing the message. Since it is refering to a property that isnt visable the error should be simular if not the same as PHPs error for accessing private/protected properties

Fatal error: Cannot access private property SpecialException::$_type in C:\path\to\exceptions.php on line 74

So changing it to
throw new Exception("Cannot access property '" . property . "' is not public");
or something of the sorts. The current error is somewhat ambiguous.

@stale stale bot added the stale Stale issue - automatically closed label Jan 9, 2019
@niden niden removed the stale Stale issue - automatically closed label Jan 9, 2019
@phalcon phalcon deleted a comment from stale bot Jan 9, 2019
@stale stale bot added the stale Stale issue - automatically closed label Apr 9, 2019
@phalcon phalcon deleted a comment from stale bot Apr 9, 2019
@stale stale bot removed the stale Stale issue - automatically closed label Apr 9, 2019
@niden niden added the enhancement Enhancement to the framework label May 16, 2019
@niden niden added 4.0 and removed 4.1 labels Jun 21, 2019
@niden niden added 4.0.1 and removed 4.1.0 labels Dec 25, 2019
niden added a commit that referenced this issue Dec 25, 2019
niden added a commit that referenced this issue Dec 25, 2019
@niden
Copy link
Member

niden commented Dec 25, 2019

Resolved in #14659

@niden niden closed this as completed Dec 25, 2019
@niden niden added 4.0.1 and removed 4.1.0 labels Dec 25, 2019
niden added a commit that referenced this issue Dec 25, 2019
niden added a commit that referenced this issue Dec 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement to the framework
Projects
Archived in project
Development

No branches or pull requests

3 participants