Skip to content

Social Links: pass an argument to get_services() to avoid errors.#7210

Merged
zinigor merged 3 commits intomasterfrom
fix/fatal-social-links-php71
May 25, 2017
Merged

Social Links: pass an argument to get_services() to avoid errors.#7210
zinigor merged 3 commits intomasterfrom
fix/fatal-social-links-php71

Conversation

@jeherve
Copy link
Copy Markdown
Member

@jeherve jeherve commented May 19, 2017

Changes proposed in this Pull Request:

PHP 7.1 returns a Fatal Error if get_services() is invoked without any argument.
Providing an argument should fix that.

Reported here:
https://wordpress.org/support/topic/publicize-and-wordpress-customizer/

Testing instructions:

  1. On a server running PHP 7.1, enable a theme using Social Links, like Ryu.
  2. Go to Appearance > Customize.
  3. Make sure you can set up Social Links without creating any errors.

Proposed changelog entry for your changes:

  • Social Links: avoid Fatal Errors on sites running PHP 7.1.

@jeherve
Social Links: pass an argument to get_services() to avoid errors.
bb63e53 
PHP 7.1 returns a Fatal Error if get_services() is invoked without any argument.
Providing an argument should fix that.
@jeherve jeherve added [Feature] Theme Tools [Pri] High [Status] Needs Review This PR is ready for review. Bug When a feature is broken and / or not performing as intended labels May 19, 2017
@jeherve jeherve self-assigned this May 19, 2017
@jeherve jeherve requested a review from aduth May 19, 2017 13:43
aduth
aduth reviewed May 19, 2017
View reviewed changes
modules/theme-tools/social-links.php Outdated
) );

foreach ( array_keys( $this->publicize->get_services() ) as $service ) {
foreach ( array_keys( $this->publicize->get_services( 'connected' ) ) as $service ) {
Copy link
Copy Markdown
Member

@aduth aduth May 19, 2017

Choose a reason for hiding this comment

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

Based on the extended description of 3bbf060 and fallback behavior of an empty argument, I'd expect this should be specified as 'all', not 'connected'.

Copy link
Copy Markdown
Member Author

@jeherve jeherve May 19, 2017

Choose a reason for hiding this comment

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

Good call! Fixed in cbe29bb

aduth
aduth approved these changes May 22, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Code looks good to me 👍

@jeherve jeherve 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 22, 2017
@eliorivero
Copy link
Copy Markdown
Contributor

eliorivero commented May 23, 2017 •
edited
Loading

I don't see why we need to pass that argument. It's actually the Publicize::get_services() function the one that could be improved by replacing this:

function get_services( $filter ) {
	if ( ! in_array( $filter, array( 'all', 'connected' ) ) ) {
		$filter = 'all';
	}

with

function get_services( $filter = 'all' ) {

and also update abstract function get_services( $filter ); to abstract function get_services( $filter = 'all' );

In this way, to request all services, we just call $this->publicize->get_services() and to get the connected ones, we call $this->publicize->get_services( 'connected' ). We save passing the parameter, creating the array, calling in_array and so on.

And if this is not clear enough, we can create a couple of intermediate functions Publicize::get_all_services() and Publicize::get_connected_services() which will in turn call Publicize::get_services() with no parameter and with connected respectively but this is not fully necessary.

I know I'm probably being a stone in the road to an easy merge, it's just that now that we're fixing this, let's fix it for good. I've created this branch https://github.com/Automattic/jetpack/compare/update/publicize-get_services-signature
If you think it's ok, I'll go ahead and close this one and open that as a proper PR.
Asking @zinigor too for his thoughts.

@eliorivero eliorivero added the [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. label May 23, 2017
@jeherve
Copy link
Copy Markdown
Member Author

jeherve commented May 24, 2017

@eliorivero That makes sense, I agree. I also like the idea of a get_all_services() and get_connected_services(); it would make it clear what services we're getting, because just calling get_services() without any arguments doesn't say much.

@zinigor
Copy link
Copy Markdown
Contributor

zinigor commented May 24, 2017

Yeah, I'm definitely for two different getter methods, because get_services( $filter ) feels weird to me. Why not get_services( $connected_only = true)?

@eliorivero
Copy link
Copy Markdown
Contributor

eliorivero commented May 24, 2017

Cool, will bring the changes from the other PR into this one.

Yeah, connected as boolean works too.

@eliorivero
Publicize: update method so it doesn't throw a notice when it's calle…
901e12f 
…d without a parameter, or particularly in PHP 7, a fatal error.
@eliorivero eliorivero force-pushed the fix/fatal-social-links-php71 branch from 2b9749a to 901e12f Compare May 24, 2017 15:30
@eliorivero eliorivero removed the [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. label May 24, 2017
@zinigor zinigor merged commit eb16b86 into master May 25, 2017
@zinigor zinigor deleted the fix/fatal-social-links-php71 branch May 25, 2017 17:15
@zinigor zinigor removed the [Status] Ready to Merge Go ahead, you can push that green button! label May 25, 2017
jeherve added a commit that referenced this pull request May 29, 2017
@jeherve
Changelog: add #7210
39b85ac
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

@aduth aduth aduth approved these changes

Assignees

@jeherve jeherve

Labels

Bug When a feature is broken and / or not performing as intended [Feature] Theme Tools [Pri] High Touches WP.com Files

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@jeherve @eliorivero @zinigor @aduth @matticbot