Skip to content

Sync: Woo: Add filters before initialising listeners#6928

Merged
enejb merged 1 commit intomasterfrom
update/woo-sync-filters
Apr 27, 2017
Merged

Sync: Woo: Add filters before initialising listeners#6928
enejb merged 1 commit intomasterfrom
update/woo-sync-filters

Conversation

@lezama
Copy link
Copy Markdown
Contributor

@lezama lezama commented Apr 5, 2017

This PR moves the extension of the whitelists to the constructor.

When we initialise all the modules, we first instantiate all sync modules and then we initialise the listeners: https://github.com/Automattic/jetpack/blob/master/sync/class.jetpack-sync-modules.php#L73

Somehow Fixes #6887

@nabsul
Copy link
Copy Markdown
Contributor

nabsul commented Apr 5, 2017

The code tests fine. Some comments:

  • I thought I read somewhere that we should avoid putting hooks in the constructor.
  • But if we're going to do that anyways, then at least let's move all the hooks there for consistency :-)
  • I found at least one place where the whitelist is still being incorrectly used: WPCOM_JSON_API_Get_Option_Endpoint::validate_input()

@lezama
Copy link
Copy Markdown
Contributor Author

lezama commented Apr 10, 2017

I found at least one place where the whitelist is still being incorrectly used: WPCOM_JSON_API_Get_Option_Endpoint::validate_input()

Should we have options whitelisted in the endpoint at all?

@nabsul
Copy link
Copy Markdown
Contributor

nabsul commented Apr 11, 2017

Should we have options whitelisted in the endpoint at all?

I think we should. Otherwise:

  • We'd be sending data that we ignore to our server (which at the moment will trigger errors/warnings in wpcom) ( @enejb to confirm)
  • It could be a security risk (sensitive data being sent over the wire)
  • Performance: Who knows what options are in the database, and what size the data is?

@enejb enejb force-pushed the update/woo-sync-filters branch from a2a9afa to 79f3633 Compare April 27, 2017 16:26
@enejb
Copy link
Copy Markdown
Member

enejb commented Apr 27, 2017

@eliorivero and @dereksmart We need to get this fix into 4.9. I have tested and it fixes the syncing of Woo Options

Copy link
Copy Markdown
Member

@enejb enejb left a comment

Choose a reason for hiding this comment

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

This change fixes the syncing of woo options as expected.

@enejb enejb merged commit 94c7e1b into master Apr 27, 2017
@matticbot matticbot removed the [Status] Needs Review This PR is ready for review. label Apr 27, 2017
@jeherve jeherve deleted the update/woo-sync-filters branch April 27, 2017 19:52
eliorivero pushed a commit that referenced this pull request Apr 28, 2017
@eliorivero
Copy link
Copy Markdown
Contributor

Cherry picked to 4.9 c133bcd

@dereksmart dereksmart modified the milestone: 4.9 Apr 28, 2017
jeherve added a commit that referenced this pull request May 10, 2017
jeherve added a commit that referenced this pull request May 29, 2017
eliorivero pushed a commit that referenced this pull request May 30, 2017
* Changelog: first pass at a changelog for 5.0

* Changelog: delete 4.9 testing list.

* Changelog: update minimum WP version to match ver. in jetpack.php

Fixes #7158

* Changelog: add #6051

* Changelog: add #6753

* Changelog: add #6928

* Changelog: add #6964

* Changelog: add #7014

* Changelog: add #7057

* Changelog: add #7060

* Changelog: add #7068

* Changelog: add #7070

* Changelog: add #7072

* Changelog: add #7071

* Changelog: add release date and post shortlink.

* Changelog: add #7094

* Changelog: add #7100

* Changelog: add #7108

* Changelog: add #7113

* Changelog: add #7123

* Changelog: add #7135

* Changelog: add #7143

* Changelog: add #7151

* Changelog: add #6996

* Changelog: add #7105

* Changelog: add #7132

* Changelog: add #7166

* Changelog: fix typo in 4.9 changelog.

* Changelog: remove older releases' changelogs.

@see p1HpG7-42e-p2

* Changelog: add #7090

* Changelog: add #7095

* Changelog: add #7112

* Changelog: add #7115

* Changelog: add #7122

* Changelog: add #7137

* Changelog: add #7138

* Changelog: add #7140

* Changelog: add #7154

* Changelog: add ##7155

* Changelog: add #7163

* Changelog: add #7167

* Changelog: add #7171

* Changelog: add #7180

* Changelog: add #7181

* Changelog: add #7183

* Changelog: add #7184

* Changelog: add #7189

* Changelog: add #7191

* Changelog: add #7193

* Changelog: add #7198

* Changelog: add #7200

* Changelog: add #7209

* Changelog: add #7212

* Testing list: add instructions for #7115

* Changelog: add #7188

* Changelog: add #7205

* Changelog: add #7225

* Changelog: add #6872

* Changelog: add #7107

* Changelog: add #7118

* Changelog: add #7142

* Changelog: add #7170

* Changelog: add #7210

* Changelog: add #7218

* Changelog: add #7232

* Changelog: add #7211

* Changelog: add #7213

* Changelog: add #7229

* Changelog: add #7230

* Changelog: add #7214

* Draft changelog for 5.0

* Changelog updates: 2nd pass at a clearer changelog.

- Fix typos.
- Use consistent tense and tone across all changelog.
- Remove unclear items.

* Changelog: add #7026

* Changelog: add #7058

* Changelog: add #7125

* Changelog: add #7249

* Changelog: add #7185

* add mentions of image widget migration

* Changelog: add info about new output for CLI command.

* Changelog: add WP version number matching the new Image Widget.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants