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

732: Fix incorrect default in for_export() function for filters. #738

Closed
wants to merge 1 commit into from

Conversation

@toolstack
Copy link
Contributor

@toolstack toolstack commented Jun 21, 2017

for_translation() already handles the default so passing in a null is fine so don't bother setting the default in for_export().

Resolves #732.

for_translation() already handles the default so passing in a null is fine so don't bother setting the default in for_export().
@toolstack toolstack requested a review from ocean90 Jun 21, 2017
@toolstack toolstack self-assigned this Jun 21, 2017
@toolstack toolstack added this to the 2.4 milestone Jun 21, 2017
@@ -299,7 +299,7 @@ public function set_fields( $db_object ) {
}

public function for_export( $project, $translation_set, $filters = null ) {

This comment has been minimized.

@ocean90

ocean90 Jun 21, 2017
Member

The default of $filters should probably be changed to an array now since that's the expected type by for_translation().

This comment has been minimized.

@toolstack

toolstack Jun 21, 2017
Author Contributor

Makes sense I'll update it later.

I think there's a problem though with my solution so it may need an update, I'll verify it before I merge.

@toolstack
Copy link
Contributor Author

@toolstack toolstack commented Jun 22, 2017

The PR simply inverts the problem, causing the "all current" export to break, closing this PR and opening a new one to resolve the issue.

@toolstack toolstack closed this Jun 22, 2017
@toolstack toolstack deleted the 732-fix-export-matching-filter branch Jun 22, 2017
@toolstack toolstack modified the milestones: 3.0, 2.4 Aug 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants