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

[Data Objects] Save default values to versions #10087

Conversation

BlackbitDevs
Copy link
Contributor

@BlackbitDevs BlackbitDevs commented Aug 19, 2021

With #5594 saving field's default value to database was introduced. The problem is that these default values do not get saved to versions currently. This is especially bad because when you publish this version, you will again have null in the respective fields.

Steps to reproduce:

  1. Create class with input field abc, default value test
  2. Create object of above class and save it
  3. In the database tables now there is test in column abc
  4. Go to versions tab, select jsut saved version -> there abc is empty
  5. Restore the just saved version -> The value is now also null in the database column -> this is really bad because you can not restore the exact state the object had at this point (what is the main purpose of versioning beside tracking changes)

With this PR the default values are also saved to the database (and also to the PHP object being saved - which is important, if you have further operations with it after saving).

@BlackbitDevs
Copy link
Contributor Author

Are the failing tests caused by this PR? I don't think so but am not sure

@brusch
Copy link
Member

brusch commented Aug 19, 2021

@BlackbitNeueMedien failing tests seem to be related, base branch runs through without any issues.

@brusch brusch added this to the 10.1.2 milestone Aug 25, 2021
@brusch brusch added the Bug label Aug 25, 2021
@weisswurstkanone
Copy link
Contributor

@BlackbitNeueMedien I guess a unit test would definitely make sense here. wdyt 😉

@weisswurstkanone
Copy link
Contributor

@BlackbitNeueMedien This needs to be done for fieldcollections, bricks & blocks as well.

@BlackbitDevs
Copy link
Contributor Author

Added test, added support for localized fields, object bricks, field collections.

@BlackbitDevs
Copy link
Contributor Author

BlackbitDevs commented Aug 27, 2021

@weisswurstkanone For the test I wanted to add an input field with a default value. How does this work? Obviously it is not enough to add it to the definition file in https://github.com/pimcore/pimcore/pull/10087/files#diff-07e12400f01e9028add55e187301705004f356fbc4a4c23ce29b2113a31d88e9R572

But CI complains about [Exception] Call to undefined method getInputWithDefault in class Pimcore\Model\DataObject\Unittest

@dvesh3
Copy link
Contributor

dvesh3 commented Aug 27, 2021

@BlackbitNeueMedien you have to add a datachild with name inputWithDefault (and call setDefaultValue) in Unittest class setup here https://github.com/pimcore/pimcore/blob/10.x/tests/_support/Helper/Model.php#L549

@weisswurstkanone
Copy link
Contributor

Thanks Divesh.

@BlackbitNeueMedien just for clarification: The classes are generated by code. The json files are just generated for your convenience.

@weisswurstkanone
Copy link
Contributor

@BlackbitNeueMedien what is the status regarding the tests ? 🤔

@weisswurstkanone weisswurstkanone modified the milestones: 10.1.2, 10.1.3 Sep 1, 2021
@BlackbitDevs
Copy link
Contributor Author

The status is that I currently do not know why the tests fail. If you have some time to support, this would be great.

@weisswurstkanone
Copy link
Contributor

I am on it! Thx!

@weisswurstkanone
Copy link
Contributor

Hello @BlackbitNeueMedien , regarding the test. I believe it should be

        $data = [
            'lblockadvancedRelations' => new BlockElement('lblockadvancedRelations', 'advancedManyToManyRelation', $reference),
        ];
        $source->setLtestblock([$data], 'de');
        $source->save();

        //link source on target
        $target = TestHelper::createEmptyObject();
        $target->setHref($source);
        $target->save(); //block data should retain here

        //update block element - manyToManyRelations
        $referenceNew = TestHelper::createEmptyObject();
        $referenceNewId = $referenceNew->getId();
        $referenceNew = new BlockElement('lblockadvancedRelations', 'advancedManyToManyRelation', $referenceNew);
        $source->getLtestblock('de')[0]['lblockadvancedRelations']->setData($referenceNew);
        $source->save();

so lblockadvancedRelations instead of blockmanyToManyRelations.

@brusch
Copy link
Member

brusch commented Oct 7, 2021

@BlackbitNeueMedien I'll close this PR since you're separating them anyway, right 😊 Looking forward to the 2 PRs.

@brusch brusch closed this Oct 7, 2021
@BlackbitDevs
Copy link
Contributor Author

@brusch No. Please reopen it. I just remove the ValidationException things from this PR here and put those changes into another one.

@brusch brusch reopened this Oct 7, 2021
@brusch
Copy link
Member

brusch commented Oct 7, 2021

@BlackbitNeueMedien sure, no problem 😊

@BlackbitDevs BlackbitDevs force-pushed the bugfix/save-default-values-to-versions branch from 30f1083 to b9de9f3 Compare October 7, 2021 13:41
@BlackbitDevs
Copy link
Contributor Author

BlackbitDevs commented Oct 7, 2021

Ok, I reverted all changes related to the ValidationExceptions. This PR is now ready for review

@BlackbitDevs
Copy link
Contributor Author

BlackbitDevs commented Oct 7, 2021

Sorry, @weisswurstkanone's fix got missing because I am too stupid to use git ;-)
Added it again.

@brusch
Copy link
Member

brusch commented Oct 12, 2021

@BlackbitNeueMedien I wouldn't introduce this change in a bug-fix release, because someone could rely on the current behavior. I think it would be ok to do this in a minor release, but before we gonna merge this PR we should discuss this as well.

In the meantime, could you please rebase this PR onto 10.x. Thanks a lot!

@brusch brusch removed this from the 10.1.5 milestone Oct 12, 2021
@brusch brusch removed the Bug label Oct 12, 2021
@BlackbitDevs BlackbitDevs changed the base branch from 10.1 to 10.x October 13, 2021 06:08
@BlackbitDevs
Copy link
Contributor Author

BlackbitDevs commented Oct 13, 2021

Rebased to 10.x.
Actually it really is a bug because you cannot restore a certain state by publishing a version like I described in #10087 (comment) - but I do not have any problem that this gets released with the next minor release.

@CLAassistant
Copy link

CLAassistant commented Oct 26, 2021

CLA assistant check
All committers have signed the CLA.

@brusch brusch added this to the 10.3.0 milestone Jan 11, 2022
@brusch
Copy link
Member

brusch commented Jan 11, 2022

@BlackbitNeueMedien one more minor thing, we should mention the behavior change in the upgrade notes. Thanks!

@BlackbitDevs
Copy link
Contributor Author

@brusch I added documentation in the upgrade notes.

@weisswurstkanone weisswurstkanone merged commit bfe097f into pimcore:10.x Jan 13, 2022
@weisswurstkanone
Copy link
Contributor

thanks a lot!

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

Successfully merging this pull request may close these issues.

None yet

6 participants