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 ORM writeAttribute when value is an associative array #14021

Closed
wajdijurry opened this issue Apr 27, 2019 · 12 comments

Comments

Projects
4 participants
@wajdijurry
Copy link

commented Apr 27, 2019

When using $model->writeAttribute('field', ['a'=>'apply', 'b'=>'banana']) to write an associative array to an attribute in the model, the result is strange.

Expected and Actual Behavior

Expected result:

$model->field = ['a'=>'apply', 'b'=>'banana']

Actual result:

$model->a = 'apple';
$model->b = 'banana';

This behavior happening when trying to write a new attribute to a model, and the value must be an associative array.

Details

  • Phalcon version: (3.4.2)
  • PHP Version: (7.2.15)
  • Operating System: Ubuntu 18.10
  • Installation type: Docker
@zsilbi

This comment has been minimized.

Copy link
Contributor

commented Apr 27, 2019

I also noticed this yesterday when I was working on other Model related issues.
I have a work-in-progress PR for this in the 4.0.x branch here: #14018

@zsilbi

This comment has been minimized.

Copy link
Contributor

commented May 2, 2019

I did some testing and this bug is confirmed, but only occurs when field in not defined as a property in Model because the problem is in Model::__set().

For example:

class Users extends Model
{
    public $id;
    public $name;
}
$user = new Users();
$user->writeAttribute('whatEverUndefinedProperty', ['id' => 123, 'name' => 'My Name']);
// or
$user->whatEverUndefinedProperty = ['id' => 123, 'name' => 'My Name'];

print_r($user->toArray());

results in:

Array
(
   [id] => 123
   [name] => My Name
)

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

@sergeyklay

This comment has been minimized.

Copy link
Member

commented May 2, 2019

Actually it is important feature and we can not just remove this behavior from 3.x branch because:

  • This behavior is documented
  • Many developers rely on this behavior
  • It will break existing applications
@zsilbi

This comment has been minimized.

Copy link
Contributor

commented May 2, 2019

@sergeyklay, I couldn't find this anywhere in the docs

@sergeyklay

This comment has been minimized.

Copy link
Member

commented May 2, 2019

As a far as I know for 4.x this should be changed in #13677

@zsilbi

This comment has been minimized.

Copy link
Contributor

commented May 2, 2019

As a far as I know for 4.x this should be changed in #13677

That PR did not solve this, the code on line 359 causes this issue.
https://github.com/borisdelev/cphalcon/blob/7b6cfe807674147e5186d2680277706766d95ac7/phalcon/mvc/model.zep#L359

@sergeyklay

This comment has been minimized.

Copy link
Member

commented May 2, 2019

Well, @wajdijurry pointed current issue for

Phalcon version: (3.4.2)

The main idea is (as I said before) that we cannot change this for the 3.x branch. I personally was a part in battles long in a months, as soon as we changed something in area of Model::__set/__get because it is most used feature. So theoretically we could change this behavior for the 4.x version (until the first beta is released), however I would like to see the real rationales for this and test coverage

@zsilbi

This comment has been minimized.

Copy link
Contributor

commented May 2, 2019

All right, I am writing tests now to cover these functions and the open issues related to them.

@wajdijurry

This comment has been minimized.

Copy link
Author

commented May 2, 2019

@sergeyklay what issue we are talking about ? I think it's a bug in Phalcon!
Why it is needed as this ? The logic says if I assigned an attribute to a model with an associative array, it should be assigned as it, not shifted up, right ?

@sergeyklay

This comment has been minimized.

Copy link
Member

commented May 2, 2019

As a far as I remember this feature was introduced in 2.x branch. So no, it is not a bug, it is well know behavior. This is why I said we can't this change for 3.x

@zsilbi

This comment has been minimized.

Copy link
Contributor

commented May 2, 2019

I think this function was meant to be working this way:

class RobotsParts extends Model
{
    public function initialize()
    {
        $this->belongsTo('robots_id', 'Robots', 'id', ['alias' => 'robot']);
    }
}

If we set the related record as an array:

$robotPart = new Models\RobotsParts();
$robotPart->robot = ['name' => 'TestRobotName'];

$robot = $robotPart->robot;

var_dump(get_class($robot));

print_r($robot->toArray());

The result should be:

string(6) "Robots"

Array
(
   [name] => "TestRobotName"
)

@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 the Enhancement label May 11, 2019

@niden niden added this to To do in 4.0 Release via automation 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.