Phalcon\Mvc\Model very strange setter behaviour if provided multidimensional array

[cottton]

Questions should go to https://forum.phalconphp.com Documentation issues should go to https://github.com/phalcon/docs/issues

Expected and Actual Behavior

Describe what you are trying to achieve and what goes wrong. Setting up a model with a simple foreach ... $this->{$key} = $value; overwrites already set fields. Totally messes up recursively.

Provide output if related

// paste output here
// expected:
    array (
        'id' => NULL,
        'name' => 'cottton',
    )
    array (
        'id' => NULL,
        'name' => 'Foo',
    )
// actual
    array (
        'id' => NULL,
        'name' => 'cottton',
    )
    array (
        'id' => NULL,
        'name' => 'shit!',
    )

Provide minimal script to reproduce the issue

CREATE TABLE `robots` (
`id` int(10) unsigned NOT NULL AUTO_INCREMENT,
    `name` varchar(70) NOT NULL,
    PRIMARY KEY (`id`)
);

class Robot extends Phalcon\Mvc\Model
{
    public function getSource()
    {
        return 'robots';
    }

    public function setData($data)
    {
        foreach ($data as $key => $value) {
            $this->{$key} = $value;
        }
        return $this;
    }
}

$robot = new \Robot();

$robot->name = 'cottton';
echo var_export($robot->toArray(), true) . PHP_EOL;  // name === cottton

$robot->setData(
    [
        'name'       => 'Foo', // actual name expected to be set.

        //problems:
        // if providing (on purpose or not) an array ...

        // with any offset (numeric, string, w/e)
        'any_offset' => [

            // and in here is the same offset as on parent (i.e. "name")
            // then robot will be named:
            'name'                        => 'FU',

            // except another array is given (on purpose or not)
            // with any offset EXCEPT the offset before (i.e. "any_offset")
            'any_offset_but_"any_offset"' => [
                'name' => 'FU2',
            ],

            // ... recursively
            'and_so_on ...'               => [
                'well ...' => [
                    'name' => 'this is fucked up',
                ],
            ],

            'and_so_on ... ...' => [
                'well 1' => [
                    'well 2' => [
                        'well 3' => [
                            'well 4' => [
                                'well 5' => [
                                    'well 6' => [
                                        'name' => 'shit!',
                                    ],
                                ],
                            ],
                        ],
                    ],
                ],
            ],
        ],
    ]
);
echo var_export($robot->toArray(), true) . PHP_EOL;// name === shit!

Details

• System info and versions (if possible): (phalcon info)

Phalcon DevTools (3.0.3)

Environment::
  OS: Linux medev-accounting-app-1 4.4.0-53-generic #74~14.04.1-Ubuntu SMP Fri Dec 2 03:43:31 UTC 2016 x86_64
  PHP Version: 7.0.11
  PHP SAPI: cli
  PHP Bin: /usr/local/bin/php
  PHP Extension Dir: /usr/local/lib/php/extensions/no-debug-non-zts-20151012
  PHP Bin Dir: /usr/local/bin
  Loaded PHP config: /usr/local/etc/php/php.ini
Versions::
  Phalcon DevTools Version: 3.0.3
  Phalcon Version: 3.0.1
  AdminLTE Version: 2.3.6

• PHP Version: (php -v) PHP 7.0.11

• Operating System: Ubuntu 14.04

• Server: Nginx | Apache | Other Nginx

• Other related info (Database, table schema): NA

[elibyy]

it happens even without the foreach $robot->random = ['name' => 'cottton']; will result the same

also, the related code

[Jurigag]

It's known problem and it's expected behaviour. As you see phalcon expects property to be relation if it's provided as a string.

[elibyy]

@Jurigag why would it be expected behavior, what if I'm holding a simple array and on save event doing stuff? should it be used with real field?

[Jurigag]

Just use setter. Magic methods are used for setting relations. As i wrote, it is a bug in my opinion, but it's expected behavior right now.

[cottton]

Any chance to get a workaround? As @elibyy mentioned the before -actions would be kind of useless: https://docs.phalconphp.com/uk/latest/reference/models.html#initializing-preparing-fetched-records

I dont see any checks if any relations has been set up. Imo it should be checked if the current model has relations (::belongsTo, ...). If no relations ever set then it should act as expected: property = value where value is array, string, int or w/e.

Looking a the code i see no chance to get this working from our side (PHP). Because all ends up in the __set method :( .

[elibyy]

@cottton i agree too, for now you can override the__set method and pass to parent::__set based on your logic

[Jurigag]

As i wrote - just use setter.

[cottton]

Just tried that ;) added

    public function __set($key, $value)
    {
        $this->{$key} = $value;
    }

But if im not calling set($key, $value) (which i cant because would get same result) what could go wrong? Obviously no relations would work. And what else?

[Jurigag]

Like this:

$model->setRandom(['name' => 'cottton']);

Will work without any problem :)

Also will give small boost performance. Always set all properties and add getters and setters

[cottton]

@Jurigag with many models and many fields this could take days to add that for every model. Also wil be a pain in the *** on upgrades/updates.

[Jurigag]

Then use IDE. Im adding setters and getters in 1-2 seconds for all propeties.

[elibyy]

@cottton you can use call_user_func_array[$this,"set{$key}"],$value)

[Jurigag]

So like you saw - not a bug, i guess current behaviour won't change and i'm fine with that. Cause you should ALWAYS set properties and use getters and setters like there is two options:

• use public properties
• use private/protected properties + getters/setters

Other solutions are your mistake(like not setting properties at all) and they are just wrong, in other languages you would have error. Just magic methods are really worst thing which could happen to php. Especially __set and __get.

[cottton]

@Jurigag No offence but you guys added the magic method here https://github.com/phalcon/cphalcon/blob/master/phalcon/mvc/model.zep#L4172 If you think this is the worst thing ever happen to PHP why it found its way into the model? :)

Anyway - found a solution: https://github.com/phalcon/cphalcon/blob/master/phalcon/mvc/model.zep#L452

    public function setData($data)
    {
        $this->assign($data);
        return $this;
    }

Atm seems to do what i expected without overwriting the phalcon model setter.

[Jurigag]

Beacause it's used for setting relations in framework, it's not supposed to be used by people. I didn't added it, it was from beginning like this. This is not solution. The solution is this as i wrote:

• use public properties
• use private/protected properties + getters/setters

Well other than that i don't know why you need this method. You can just call assign without this method, but this is not a solution. YOU MUST HAVE PROPERTIES IN YOUR CLASSES ANYWAY, as well as setters if they are private/protected. If you don't have then it's your mistake and your code is shit.

[cottton]

it's not supposed to be used by people oO? _if you meant the _set method then forget the rest below

Assigns values to a model from an array Its public function.

I see no relation -related code in there. This seems to be a very "extended" version of the setData method im using. Data, mapping, whitelist. Actually this code https://github.com/phalcon/cphalcon/blob/master/phalcon/mvc/model.zep#L452 does exactly what i need.

[Jurigag]

Then use it. Just remove this setData and use assign alone :)

I mean you should not use __set or any other magic thing for your app to work. You should have public properties and/or getters/setters anyway. Using __set to setting your properties is just bad architecture and it's mistake.

[nadorator]

using setter/getter or public properties are bad OO as well because the object should not expose its data implémentation and should avoid long process of code refactoring. Saying code is a shit to someone else without knoledge of OO is not very professional. A setData method is the most effective solution for OO and maintainability