gabordemooij / redbean

ORM layer that creates models, config and database on the fly
https://www.redbeanphp.com
2.31k stars 279 forks source link

Changing fields in own lists unlinks / deletes the own bean, then re-links / re-inserts the bean #840

Closed MangoMarcus closed 4 years ago

MangoMarcus commented 4 years ago

When you store a bean RedBean uses Repository::processGroups() to determine which of the own beans are additions, which have been removed (trashcan), and which have remained (residue):

protected function processGroups( $originals, $current, $additions, $trashcan, $residue )
{
    return array(
        array_merge( $additions, array_diff( $current, $originals ) ),
        array_merge( $trashcan, array_diff( $originals, $current ) ),
        array_merge( $residue, array_intersect( $current, $originals ) )
    );
}

The criteria for calculating these differences seems like a bug to me though. The PHP manual for both array_diff() and array_intersect() states that:

Two elements are considered equal if and only if (string) $elem1 === (string) $elem2. That is, when the string representation is the same.

Casting a bean to a string returns a JSON string of the properties (see OODBBean::__toString()), meaning that Repository::processGroups() is actually comparing the beans as JSON.

You can see this yourself by doing something like this:

class Pupil
{
    public $name;

    public function __construct( $name )
    {
        $this->name = $name;
    }

    public function __toString()
    {
        echo 'toString! - '.$this->name.PHP_EOL;
        return $this->name;
    }

}

$a = new Pupil( 'Bob' );
$b = new Pupil( 'Alice' );
$c = new Pupil( 'Jane' );

print_r( array_diff(
    [ $a, $b ],
    [ $b, $c ]
) );

Prints:

toString! - Alice
toString! - Jane
toString! - Bob
toString! - Alice
Array
(
    [0] => Pupil Object
        (
            [name] => Bob
        )

)

I can only assume this is an oversight, because obviously changing any field would result in different JSON, which makes Repository::processGroups() think that it isn't the same bean. Even differences between data types ie. strings vs ints/floats would have the same effect.

RedBean then either deletes the own bean or sets the parent ID to NULL (depending on if it's xown or not). In the example below pupil 123 would be deleted (assuming it's previously been stored):

$teacher->xownPupil[ 123 ]->name = 'Bob';
R::store( $teacher );

I wonder if Repository::processGroups() was instead supposed to use array_diff_key() and array_intersect_key() - though I guess that would depending on the beans being ID-indexed, which I'm not sure is always the case.

It might be worth considering instead using array_udiff() and array_uintersect(), something like:

protected function processGroups( $originals, $current, $additions, $trashcan, $residue )
{
    $compare = function( $a, $b ) {
        return $a->equals( $b );
    };

    return array(
        array_merge( $additions, array_udiff( $current, $originals, $compare ) ),
        array_merge( $trashcan, array_udiff( $originals, $current, $compare ) ),
        array_merge( $residue, array_uintersect( $current, $originals, $compare ) )
    );
}

Let me know if I'm missing something, cheers!

Lynesth commented 4 years ago

I'm not sure I understand, but I'm a bit tired, so I simply tried to do this:

$user = R::dispense('user');
$user->name = 'John';
R::store($user);

$thing = R::dispense('thing');
$thing->name = 'That';

$user->xownThing[] = $thing;
R::store($user);
var_dump($user->xownThing[1]->name); // That

$user->xownThing[1]->name = 'This';
R::store($user);
var_dump($user->xownThing[1]->name); // This

Everything behaves as it should as far as I can tell or is it not working for you?

Or are you saying that we end up removing and then adding back the modified bean to the list and that it is a waste of time?

gabordemooij commented 4 years ago

Hi there,

You can find a warning about overriding __toString here: https://redbeanphp.com/index.php?p=/one_to_many

I can only assume this is an oversight, because obviously changing any field would result in different JSON

No, because beans are objects and therefore they are passed by reference, the mechanism allows you to use a different object than the original if the string representation matches, you can use this to your advantage.

If you encounter a scenario where this behavior causes a problem, please share a test case with us so we can look into it.

cheers, Gabor

MangoMarcus commented 4 years ago

Thanks both!

I've realised it was actually due to the sys.shadow not being updated properly. I didn't realise that seems to be updated globally when a bean's field is changed. Because it wasn't keeping a fresh copy of the own bean's fields, it didn't recognise it as the same child.

It's possibly user error on my part then or that I was using an older version of RedBean, I'll close after I'll done a little more digging

Cheers