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

Mark the last key not dirty?! #4999

Open
mike4git opened this issue Sep 20, 2019 · 10 comments

Comments

@mike4git
Copy link
Contributor

commented Sep 20, 2019

In the ObjectBricks loading mechanism there is an error we were stumbling over.

In line

$brick->markFieldDirty($key, false);

the last key will be marked as NOT dirty but we are sure it should have been propably the object itself:
$brick->markFieldDirty('_self', false);

@mike4git mike4git changed the title Mark the last key dirty?! Mark the last key not dirty?! Sep 20, 2019
mike4git added a commit to mike4git/pimcore that referenced this issue Sep 20, 2019
@weisswurstkanone

This comment has been minimized.

Copy link
Contributor

commented Sep 23, 2019

please provide the build number. thx!

@mike4git

This comment has been minimized.

Copy link
Contributor Author

commented Sep 23, 2019

Build number: v5.8.4 (which I have used for fixing)
But we guess that the following commit
a051862#diff-30cd79ff8a376b104bc4bd82d778d0f7L99
has introduced this bug.

@mike4git mike4git closed this Sep 23, 2019
@mike4git mike4git reopened this Sep 23, 2019
@weisswurstkanone

This comment has been minimized.

Copy link
Contributor

commented Sep 23, 2019

I would say already fixed via
#4677

@mike4git

This comment has been minimized.

Copy link
Contributor Author

commented Sep 23, 2019

In Issue #4677 you have corrected the saving of objects but not the loading.
And during the loading there should be no field marked as dirty, as well as the object itself.
But the markFieldDirty('_self') would not be deleted, if you would not use my change.
And the current code in the loading method is marking always the LAST $key as not dirty.
I would say this is development by accident, isn't it?!

  1. The change should not happen in the save BUT in the load method.
  2. The wrong line of code uses $key outside of the foreach block where it has been defined. (s. line 111 above)
@weisswurstkanone

This comment has been minimized.

Copy link
Contributor

commented Sep 23, 2019

still ... could you please try to reproduce this with at least the latest pimcore 5 build. please note that 5.8.4 is kind of outdated :-)

@mike4git

This comment has been minimized.

Copy link
Contributor Author

commented Sep 23, 2019

Nice try...but this didn't work ;-)

In the current version (master and 6.2.1 - https://github.com/pimcore/pimcore/blob/master/models/DataObject/Objectbrick/Dao.php) you can see the following code block (for convenience reason I've extracted it here):

public function load(DataObject\Concrete $object, $params = [])
{
        $fieldDef = $object->getClass()->getFieldDefinition($this->model->getFieldname());
        $values = [];
        ...
                foreach ($fieldDefinitions as $key => $fd) {
                    ...
                } // end of foreach, where $key has been defined


                $setter = 'set' . ucfirst($type);
                if ($brick instanceof DataObject\DirtyIndicatorInterface) {

                    // here $key is used again OUTSIDE of foreach
                    $brick->markFieldDirty($key, false);
                }
       
        ...
}

And because of that block our "last" object brick relation to our products have been deleted and not inserted again.

Please read the code, get an idea of what has gone wrong and take one of my two fixes.

@weisswurstkanone

This comment has been minimized.

Copy link
Contributor

commented Sep 23, 2019

ok, could you please create a PR for pimcore 6 ? thanks a lot!

@weisswurstkanone

This comment has been minimized.

Copy link
Contributor

commented Sep 23, 2019

as well as steps to reproduce on the pimcore demo instance please ;-)

@mike4git

This comment has been minimized.

Copy link
Contributor Author

commented Sep 23, 2019

I did it: #5001
But reproducing steps are NOT easily possible.

@weisswurstkanone

This comment has been minimized.

Copy link
Contributor

commented Sep 23, 2019

Also not able to reproduce it. However, you may have a valid point - which IMHO does not explain what you are describing here. BTW, I think we should replace it with resetDirtyMap because after loading the brick should be clean anyway.

Please don't forget to sign the CLA.

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