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

Model relation reusable=>false not working #13531

Closed
nilsm opened this issue Oct 16, 2018 · 10 comments

Comments

Projects
4 participants
@nilsm
Copy link

commented Oct 16, 2018

Expected and Actual Behavior

I have a model with a relation set to 'reusable' => false.
I would expect the relation to be fetched from the database every time it is accessed.
However the relation is cached after the first call, even though 'reusable' => false.
Using 'getRelated()' to access the relation works as intended.

Note: 'reusable' => false is the default setting, so models do not work as intended by default.

Here is some sample code:

class Robots extends \Phalcon\Mvc\Model
{
    public $id;
    public $name;
    public function initialize()
    {
        $this->hasMany("id", "Parts", "robot_id", ['reusable' => false]);
    }
}

class Parts extends \Phalcon\Mvc\Model
{
    public $id;
    public $robot_id;
    public $name;
    public static function find($parameters = NULL)
    {
    	echo 'find';
    	return parent::find($parameters);
    }
    public function initialize()
    {
        $this->belongsTo("robot_id", "Robots", "id", ['alias' => 'robot']);
    }
}

// actual code to test
$robot = new Robot();
$robot->name = 'Bob';
$robot->save();

$robot->parts; // first query - echos 'find'
$robot->parts; // 2nd query - no echo, even though it should re-query
$robot->getRelated('parts'); // echo 'find', as intended
$robot->getRelated('parts'); // echo 'find', as intended

Severity

Critical - this is stopping us from upgrading to Phalcon 3 + model relations do not work as documented.

Details

  • Phalcon version: 3.4.1
  • PHP Version: 5.6
  • Operating System: n/a
  • Installation type: Compiling from source
  • Zephir version (if any): n/a
  • Server: n/a
  • Other related info
CREATE TABLE `robots` (
    `id` int(10) unsigned NOT NULL AUTO_INCREMENT,
    `name` varchar(70) NOT NULL,
    PRIMARY KEY (`id`)
);
CREATE TABLE `parts` (
    `id` int(10) unsigned NOT NULL AUTO_INCREMENT,
    `robot_id` int(10) unsigned NOT NULL,
    `name` varchar(70) NOT NULL,
    PRIMARY KEY (`id`),
    FOREIGN KEY (robot_id) references robots(id)
);
@nilsm

This comment has been minimized.

Copy link
Author

commented Oct 16, 2018

I've dug around in the source code and am pretty sure that this is the cause of the problem:

https://github.com/phalcon/cphalcon/pull/11983/files

Once accessed, the related records are stored as an instance property and returned every time, regardless of what 'reusable' is set to.

@nilsm nilsm changed the title Model relation reusable not working Model relation reusable=>false not working Oct 16, 2018

@Jurigag

This comment has been minimized.

Copy link
Member

commented Oct 16, 2018

Why it should requery? After first getting it it adds as a property, this is intended to keep behavior with magic properties. You can use getParts for example(and you imh should).

@nilsm

This comment has been minimized.

Copy link
Author

commented Oct 16, 2018

@Jurigag I would expect it to requery because reusable is false. I do not see from the documentation that some access methods are intended to behave differently to others. Also in Phalcon 2 it does requery when using the magic getter and I suspect the change in behaviour was an unintended consequence of the pull request I linked to?

@nilsm

This comment has been minimized.

Copy link
Author

commented Oct 17, 2018

Just to add more info:

getParts() and getRelated('Parts') both respect the 'reusable' setting in the relation.
$robot->Parts does not - it never refetches after first access

This is a change compared to behaviour in Phalcon 2. It was introduced by, I think, #11983 - and I think it's most likely unintentional and undocumented.

@Jurigag

This comment has been minimized.

Copy link
Member

commented Oct 18, 2018

I think this is intentional to keep behavior with magic properties etc.

@nilsm

This comment has been minimized.

Copy link
Author

commented Oct 19, 2018

Thanks Jurigag - what do you think about the way it's changed from Phalcon 2 to Phalcon 3? It does requery (depending on the reusable setting) in Phalcon 2, even when using ->Parts. This is causing lots of upgrade problems for us - but it also makes me think that it's a bug, as the behaviour has changed without being documented.

@Jurigag

This comment has been minimized.

Copy link
Member

commented Oct 22, 2018

I think as i already wrote, this change was made to keep behavior how magic properties works in php world, like in 90% of magic properties stuff, when first access them __get is fired and on later accessing the same property we already use property we added in __get.

@CameronHall

This comment has been minimized.

Copy link
Member

commented Nov 27, 2018

I disagree @Jurigag. I noticed this when I created the PR for the isRelationshipLoaded method. Perhaps I misunderstand what the reusable attribute is meant to do on relationships, but my assumption would be that instance of the relationship would never be cached if "reusable" is false.

@stale stale bot added the stale label Feb 25, 2019

@CameronHall

This comment has been minimized.

Copy link
Member

commented Feb 25, 2019

@niden this is a bug

@stale stale bot removed the stale label Feb 25, 2019

zsilbi pushed a commit to zsilbi/cphalcon that referenced this issue Apr 26, 2019

@zsilbi zsilbi referenced this issue Apr 26, 2019

Closed

[WIP] Mvc\Model related storage #14018

2 of 4 tasks complete

zsilbi pushed a commit to zsilbi/cphalcon that referenced this issue May 2, 2019

@zsilbi zsilbi referenced this issue May 2, 2019

Merged

Mvc\Model::__get() / __set() and related storage #14035

4 of 4 tasks complete

zsilbi pushed a commit to zsilbi/cphalcon that referenced this issue May 2, 2019

@niden niden added this to To do in 4.0 Release via automation May 11, 2019

@niden niden added the Bug - Medium label May 11, 2019

@phalcon phalcon deleted a comment from stale bot May 11, 2019

@niden

This comment has been minimized.

Copy link
Member

commented May 11, 2019

Resolved in #14035

@niden niden closed this May 11, 2019

4.0 Release automation moved this from To do to Done May 11, 2019

@niden niden added the 4.0 label Jun 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.