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

Export code cleanup - only construct one metadata array with all types of metadata #13126

Merged
merged 4 commits into from
Nov 21, 2018

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Nov 19, 2018

Overview

Further export code cleanup
Before

Code more brittle

After

Code more maintainable

Technical Details

This moves away from the construction of multiple related arrays that need to be kept in sync to a single array holding the data for the various arrays

Comments

@mattwire this is a little bit harder to read code-wise than some of the other chunks but it is very heavily tested

Follow on currently in https://github.com/civicrm/civicrm-core/compare/master...eileenmcnaughton:export_more_im?expand=1

@civibot
Copy link

civibot bot commented Nov 19, 2018

(Standard links)

@civibot civibot bot added the master label Nov 19, 2018
@eileenmcnaughton eileenmcnaughton force-pushed the export_more branch 3 times, most recently from 847abb8 to 39d15be Compare November 19, 2018 05:59
@eileenmcnaughton eileenmcnaughton changed the title Export more Export code cleanup - only construct one metadata array with all types of metadata Nov 19, 2018
Copy link
Contributor

@mattwire mattwire left a comment

Choose a reason for hiding this comment

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

I've tested this with a contact export, contribution export and membership export. All seems to work as before.
As an aside I noticed that some membership columns were showing field names instead of labels: max_related, membership_recur_id, member_is_override, member_auto_renew but that's independent of this PR as it was happening before/after.

/**
* @var array
*/
protected $outputSpecification = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

A bit picky, but could we have a basic comment here explaining what this array is for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah - I'll add in a follow up

@eileenmcnaughton
Copy link
Contributor Author

thanks @mattwire merging based on the above

@eileenmcnaughton eileenmcnaughton merged commit 3338d96 into civicrm:master Nov 21, 2018
@eileenmcnaughton eileenmcnaughton deleted the export_more branch November 21, 2018 21:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants