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

FIX: Duplicating many_many relationships looses the extra fields (fixes #7973) #8009

Merged
merged 2 commits into from
Apr 18, 2018
Merged

FIX: Duplicating many_many relationships looses the extra fields (fixes #7973) #8009

merged 2 commits into from
Apr 18, 2018

Conversation

UndefinedOffset
Copy link
Contributor

Fixes issue when you duplicate a DataObject with a many_many relationship with extra fields defined for the relationship extra fields are lost due to how DataObject::duplicateManyManyRelations() and DataObject::duplicateRelations() function. In short when the new items are added to the many_many relationship the extra fields are not included or even retrieved when DataObject::duplicateRelations() adds the item to the list. This issue also seems to affect 4.0 but the fix required would be slightly different see #7973 for details.

@dhensby
Copy link
Contributor

dhensby commented Apr 17, 2018

This needs to go into 3, I'm afraid

@dhensby dhensby changed the base branch from 3.6 to 3 April 17, 2018 15:58
@dhensby
Copy link
Contributor

dhensby commented Apr 17, 2018

If you can find a way of patching duplicateRelations without changing the API surface, then it could make a patch release.

@dhensby
Copy link
Contributor

dhensby commented Apr 17, 2018

I reckon you can add a new condition to duplicateRelations which was:

if ($relations instanceof ManyManyList) and then use your logic to populate extra data (line 589)

@UndefinedOffset
Copy link
Contributor Author

Ya that would make sense, let me give it a shot and I'll update in a bit.

@UndefinedOffset
Copy link
Contributor Author

@dhensby There hows that look? Also should we back port this to 3.5 too or just for 3.6 and another solution for 4.0?

@UndefinedOffset
Copy link
Contributor Author

Not sure why the PGSQL test failed... it should have worked my local copy with postgres 9.3 and SilverStripe 3.6.5 :S passed

@kinglozzer
Copy link
Member

@UndefinedOffset Try adding a sort, I seem to remember PostgreSQL having issues with inconsistent result ordering if you don’t provide one...

@dhensby
Copy link
Contributor

dhensby commented Apr 18, 2018

It looks good to me, let's rebase onto 3.5 :)

@dhensby dhensby changed the base branch from 3 to 3.5 April 18, 2018 11:14
@UndefinedOffset
Copy link
Contributor Author

Will do and I have a branch for 4.0 I'll open a pull for that too https://github.com/webbuilders-group/silverstripe-framework/tree/duplicate-many-many-fix-4-0

@dhensby dhensby merged commit f30cd61 into silverstripe:3.5 Apr 18, 2018
@UndefinedOffset UndefinedOffset deleted the duplicate-many-many-fix branch April 18, 2018 20:33
dhensby added a commit to dhensby/silverstripe-framework that referenced this pull request Jun 4, 2018
dhensby added a commit to dhensby/silverstripe-framework that referenced this pull request Jun 4, 2018
dhensby added a commit to dhensby/silverstripe-framework that referenced this pull request Jun 4, 2018
robbieaverill added a commit that referenced this pull request Jun 5, 2018
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.

None yet

3 participants