Skip to content

Photon: Defensive coding on filtered function#7090

Merged
eliorivero merged 2 commits intomasterfrom
fix/7075
May 15, 2017
Merged

Photon: Defensive coding on filtered function#7090
eliorivero merged 2 commits intomasterfrom
fix/7075

Conversation

@kraftbj
Copy link
Copy Markdown
Contributor

@kraftbj kraftbj commented Apr 28, 2017

Pass default values to account for others applying the filter without passing all four args.

Every place we use these variables are already checked to ensure they are set, so there should be no harm in nulling them.

Fixes #7075

Testing instructions:

  • Use the Fludia theme at https://wordpress.org/themes/fluida/
  • Without patch, see PHP Warnings in the error log
  • With patch, no errors.
  • Verify other image content (on that theme or any other) is still applying Photon values

@kraftbj kraftbj added [Feature] Photon aka "Image CDN". Feature developed in the Image CDN package and shipped in multiple plugins [Status] Needs Review This PR is ready for review. Bug When a feature is broken and / or not performing as intended labels Apr 28, 2017
@kraftbj kraftbj requested a review from dereksmart April 28, 2017 14:19
@kraftbj kraftbj mentioned this pull request Apr 28, 2017
Closed
@dereksmart
Copy link
Copy Markdown
Contributor

dereksmart commented Apr 28, 2017 •
edited
Loading

Rather than using null, why not use defaults that are representative of what we're expecting?

$sources = array(), $size_array = array(), $image_src = '', $image_meta = array()

Also, looks like we'll probably want a shim early in the function to check if sources is passed an array. Adding apply_filters( 'wp_calculate_image_srcset', true ); (technically incorrect, but themes obviously do this) to check if someone is blocking img srcset in a theme functions.php still makes the foreach cry.

if ( ! is_array( $sources ) ) return $sources;?

@kraftbj
Copy link
Copy Markdown
Contributor Author

kraftbj commented Apr 28, 2017

We can use array() too. I went with null since it would return false for any isset checks, but the isset checks we make are for specific array keys, so fine either way. I'll update.

No objection to the shim and makes sense.

Both changes committed.

dereksmart
dereksmart approved these changes May 4, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@dereksmart dereksmart left a comment

Choose a reason for hiding this comment

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

LGTM

@dereksmart dereksmart added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels May 4, 2017
@eliorivero eliorivero merged commit 261e665 into master May 15, 2017
@eliorivero eliorivero deleted the fix/7075 branch May 15, 2017 11:22
@eliorivero eliorivero removed the [Status] Ready to Merge Go ahead, you can push that green button! label May 15, 2017
jeherve added a commit that referenced this pull request May 23, 2017
@jeherve
Changelog: add #7090
93e5cda
jeherve added a commit that referenced this pull request May 29, 2017
@jeherve
Changelog: add #7090
4926d91
eliorivero pushed a commit that referenced this pull request May 30, 2017
@jeherve @eliorivero
Changelog: first pass at a changelog for 5.0 (#7116)
eee27f0 
* 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

Reviewers

@dereksmart dereksmart dereksmart approved these changes

Assignees

No one assigned

Labels

Bug When a feature is broken and / or not performing as intended [Feature] Photon aka "Image CDN". Feature developed in the Image CDN package and shipped in multiple plugins [Pri] High

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@kraftbj @dereksmart @jeherve @eliorivero