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

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

CameronHall opened this issue Oct 10, 2018 · 6 comments
Assignees
Labels

Comments

@CameronHall
Copy link
Member

@CameronHall 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

This comment has been minimized.

Copy link

@JABirchall JABirchall commented Oct 10, 2018

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

This comment has been minimized.

Copy link
Member Author

@CameronHall CameronHall commented Oct 10, 2018

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

This comment has been minimized.

Copy link

@JABirchall 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

This comment has been minimized.

Copy link
Member

@niden 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

This comment has been minimized.

Copy link

@JABirchall JABirchall commented Oct 10, 2018

@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 label Jan 9, 2019
@niden niden removed the stale label Jan 9, 2019
@phalcon phalcon deleted a comment from stale bot Jan 9, 2019
@stale stale bot added the stale label Apr 9, 2019
@phalcon phalcon deleted a comment from stale bot Apr 9, 2019
@stale stale bot removed the stale label Apr 9, 2019
@niden niden added the enhancement label May 16, 2019
@niden niden added this to To do in 4.1 Release via automation May 16, 2019
@niden niden added 4.0 4.1.0 and removed 4.1.0 4.0 labels Jun 21, 2019
@ruudboon ruudboon added this to Backlog in Phalcon Roadmap Dec 24, 2019
@niden niden removed this from To do in 4.1 Release Dec 24, 2019
niden added a commit to niden/cphalcon that referenced this issue Dec 25, 2019
…nged the exception text
@niden niden moved this from Backlog to Current Sprint (Ends January 10th) in Phalcon Roadmap Dec 25, 2019
@niden niden moved this from Current Sprint (Ends January 10th) to Working on it in Phalcon Roadmap Dec 25, 2019
@niden niden added 4.0.1 and removed 4.1.0 labels Dec 25, 2019
niden added a commit to niden/cphalcon that referenced this issue Dec 25, 2019
niden added a commit to niden/cphalcon that referenced this issue Dec 25, 2019
@niden niden removed the 4.0.1 label Dec 25, 2019
@niden niden added the 4.1.0 label Dec 25, 2019
@niden niden mentioned this issue Dec 25, 2019
4 of 5 tasks complete
niden added a commit that referenced this issue 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

This comment has been minimized.

Copy link
Member

@niden niden commented Dec 25, 2019

Resolved in #14659

@niden niden closed this Dec 25, 2019
@niden niden moved this from Working on it to Implemented in Phalcon Roadmap Dec 25, 2019
@niden niden added 4.0.1 and removed 4.1.0 labels Dec 25, 2019
niden added a commit to niden/cphalcon that referenced this issue Dec 25, 2019
…nged the exception text
niden added a commit to niden/cphalcon that referenced this issue Dec 25, 2019
niden added a commit to niden/cphalcon that referenced this issue Dec 25, 2019
niden added a commit that referenced this issue Dec 25, 2019
…e exception text
niden added a commit that referenced this issue Dec 25, 2019
niden added a commit that referenced this issue Dec 25, 2019
@niden niden moved this from Implemented to Released in Phalcon Roadmap Jan 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Phalcon Roadmap
  
Released
3 participants
You can’t perform that action at this time.