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

DBComposite::writeToManipulation() is never called #10913

Merged
merged 3 commits into from
Aug 14, 2023

Conversation

lekoala
Copy link
Contributor

@lekoala lekoala commented Aug 10, 2023

Fixes #8800

Credits to #9182

@GuySartorelli
Copy link
Member

Hi @lekoala, thanks for submitting this.
Can you please just briefly describe what this change is actually doing? I've not seen these constants before so it will probably help to get some perspective before I do a proper review.

Can you please also target the 4.13 branch if this is unlikely to cause regressions?

@lekoala
Copy link
Contributor Author

lekoala commented Aug 10, 2023

hi @GuySartorelli you can check here #8800 for the long explanation
i will retarget at 4.13

@lekoala lekoala changed the base branch from 5 to 4.13 August 10, 2023 10:43
@lekoala lekoala changed the base branch from 4.13 to 5 August 10, 2023 10:43
@lekoala lekoala changed the base branch from 5 to 4.13 August 10, 2023 11:48
@lekoala
Copy link
Contributor Author

lekoala commented Aug 10, 2023

ok changed to 4.13

@GuySartorelli
Copy link
Member

Touched the commit (no changes, just updated the hash) and force pushed to get CI running.

@lekoala
Copy link
Contributor Author

lekoala commented Aug 11, 2023

not sure about the ci failing, doesn't seem realated to my changes

@GuySartorelli
Copy link
Member

SilverStripe\ORM\Tests\DBCompositeTest::testSetFieldDynamicPropertyException
Failed asserting that exception of type "SilverStripe\ORM\Tests\InvalidArgumentException" is thrown.

That is a test this PR is adding - looks like it was intended to work with one of the php 8.2 comparability changes for CMS 5.0 so it's not a huge surprise it's failing here. You'll need to update or remove that test as appropriate.

@lekoala
Copy link
Contributor Author

lekoala commented Aug 11, 2023

@GuySartorelli i've commented the code, i think it makes sense to restore it once merged to ss5

tests/php/ORM/DBCompositeTest.php Outdated Show resolved Hide resolved
Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good and makes sense. Thanks!

@GuySartorelli GuySartorelli merged commit cdbc50c into silverstripe:4.13 Aug 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DBComposite::writeToManipulation() is never called
2 participants