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

GDPR replacements from JSON file #25

Closed
wants to merge 0 commits into from
Closed

Conversation

Jancis
Copy link
Contributor

@Jancis Jancis commented Oct 12, 2018

This allows GDPR replacement reading pattern from a JSON file. Editing mysql ini is a bit inconvenient so I made an option to read those from file.
You can use the same JSON structure you would for the --gdpr-replacements option.

Copy link
Contributor

@bomoko bomoko left a comment

Choose a reason for hiding this comment

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

Thanks so much for this PR.
It's good stuff - I think that the double reading of the gdpr-replacements-file should come out though. Please let me know if my comments aren't clear enough.

@@ -343,6 +354,7 @@ protected function getDumpSettingsDefault()
] + [
'gdpr-expressions' => null,
'gdpr-replacements' => null,
'gdpr-replacements-file' => null,
Copy link
Contributor

Choose a reason for hiding this comment

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

So it's not obvious, but this is possibly the reason you're rereading the file contents of your gdpr-replacements-file in /src/MysqldumpGdpr.php.

If you remove this line, you can also remove the lines 38-42 in MysqldumpGdpr.php.

@@ -35,6 +35,11 @@ public function __construct(
unset($dumpSettings['gdpr-replacements']);
}

if (array_key_exists('gdpr-replacements-file', $dumpSettings)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You're actually doing double work for yourself here - since you've already read and have passed in the data via the $dumpsettings variable. So I'd say, let's remove this as well.

@Jancis
Copy link
Contributor Author

Jancis commented Oct 15, 2018

Thank You for the review! Suggested fix added

@Jancis
Copy link
Contributor Author

Jancis commented Oct 15, 2018

I have to close and reopen this one since i shouldn't have created PR from master

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.

2 participants