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

[BUG] Models: setter did not accept array #13661

Closed
borisdelev opened this Issue Dec 13, 2018 · 7 comments

Comments

Projects
3 participants
@borisdelev
Copy link
Contributor

borisdelev commented Dec 13, 2018

Hello :)
Consider this (if we follow phalcon setter docs):

# MODEL
namespace Models;

class Lessons extends \Phalcon\Mvc\Model
{
    public $id;
    protected $title;

    public function setBobi($test)
    {
        echo $test;
        exit;

        $this->title = $test;
    }
# Somewhere
$model = new MyModel();

$model->bobi = 'soheil'; // Will print 'soheil' (see model exit)

So this will exit on 'soheil'. But if we make setter that mus accept array- it is not possible (without logick). Example:

# MODEL
namespace Models;
class Lessons extends \Phalcon\Mvc\Model
{
    public $id;
    protected $title;

    public function setBobi(array $test)
    {
        var_dump($test);
        exit;

       $this->title = implode(',', $test);
    }

end execute:

# Somewhere
$model = new MyModel();

$model->bobi = ['some', 'other']; // Will do nothing

...this will not var_dump. Why?

Will work if i use setter like function $model->setBobi(['some']); but why? If i make $model->save($post) and in post i have multiple values on some property will not work with setter.

Expected behavior

# MODEL
namespace Models;
class Lessons extends \Phalcon\Mvc\Model
{
    public $id;
    protected $title;

    public function setTitle(array $test)
    {
       $this->title = implode(',', $test);
    }
$model = new MyModel();
$model->title = ['some', 'other'];
$model->save(); // Save `title` in database == 'some,other'

Details

  • Phalcon version: all
  • PHP Version: 7.2

Thanks. I hope u understand me and sorry for my English skills.

@borisdelev

This comment has been minimized.

Copy link
Contributor Author

borisdelev commented Dec 13, 2018

I find the issue in Model (Zephir code)- if we see code from https://github.com/phalcon/cphalcon/blob/master/phalcon/mvc/model.zep and go to the line 4506 - this is the solution, but it is placed in wrong place and never work, must be before line 4475.

So, can someone fix that? Thank u!!!!

@borisdelev

This comment has been minimized.

Copy link
Contributor Author

borisdelev commented Dec 13, 2018

Or other solution is to move content from line 4503 to line after 4500. Is it better?

@borisdelev borisdelev changed the title Model setter did not accept array [BUG] Model setter did not accept array Dec 13, 2018

@borisdelev borisdelev changed the title [BUG] Model setter did not accept array [BUG] Models: setter did not accept array Dec 13, 2018

@CameronHall

This comment has been minimized.

Copy link
Member

CameronHall commented Dec 13, 2018

@borisdelev makes sense. Would line 4454 make more sense though before the object handling?

@niden is there any specific reason for the order of operations? Handle object, array then check if it has a setter?

@borisdelev

This comment has been minimized.

Copy link
Contributor Author

borisdelev commented Dec 20, 2018

I think- i can do it, but confirm which way is better? (and which branch- 4?)

@CameronHall

This comment has been minimized.

Copy link
Member

CameronHall commented Dec 20, 2018

That'd be awesome! Yes, v4 branch please :)

borisdelev added a commit to borisdelev/cphalcon that referenced this issue Dec 20, 2018

@borisdelev

This comment has been minimized.

Copy link
Contributor Author

borisdelev commented Dec 20, 2018

Well... i am not sure if i change the order is good idea, so i think this is better way: borisdelev@d8540a4#diff-95da5fc282ff73cf517e37ee54f1613b - what uve thinking?

@borisdelev borisdelev referenced this issue Dec 21, 2018

Merged

4.0.x #13677

2 of 3 tasks complete
@niden

This comment has been minimized.

Copy link
Member

niden commented Dec 21, 2018

Honestly I always hated the magic methods. As we see here since we have them, we have to try and find whether this is a relationship/model or a property or a setter.... Big mess.

At some point in the future this will need to be cleaned up (the whole ORM).

niden added a commit that referenced this issue Dec 21, 2018

@borisdelev borisdelev closed this Dec 21, 2018

@niden niden added this to To do in 4.0 Release via automation Dec 21, 2018

@niden niden added the Bug - Medium label Dec 21, 2018

@niden niden moved this from To do to Done in 4.0 Release Dec 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment