Skip to content

Fix display of enabled custom share services#6355

Merged
dereksmart merged 2 commits intomasterfrom
fix/custom-share-options
Feb 10, 2017
Merged

Fix display of enabled custom share services#6355
dereksmart merged 2 commits intomasterfrom
fix/custom-share-options

Conversation

@thingalon
Copy link
Copy Markdown
Member

@thingalon thingalon commented Feb 10, 2017

Fixes #6348

Changes proposed in this Pull Request:

  • When a missing button_style option is detected, don't clobber all options with global defaults. Merge the current options with the global defaults to ensure button_style exists, while preserving share options. This prevents the custom share details from getting deleted.
  • When closing the "Add new service" modal, reload the page directly rather than relying on regexp-based URL changes which appear to fail.

Testing instructions:

Proposed changelog entry for your changes:

  • Bugfix: custom sharing services are now rendered correctly when enabled.

Mark George added 2 commits February 10, 2017 03:54
Reload the page directly after closing the 'new sharing service' moda…
d140b5b 
…l; it was previously reloading by changing the URL based on an old pre-AJAX URL schema.
Don't clobber share settings when button_styles are missing; merge de…
87965a6 
…fault global settings with custom settings to prevent overwriting custom share details
@thingalon thingalon added [Pri] BLOCKER [Status] Needs Review This PR is ready for review. Bug When a feature is broken and / or not performing as intended labels Feb 10, 2017
@thingalon thingalon self-assigned this Feb 10, 2017
@thingalon thingalon requested a review from jeherve February 10, 2017 04:00
@zinigor zinigor 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 Feb 10, 2017
zinigor
zinigor approved these changes Feb 10, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@zinigor zinigor left a comment

Choose a reason for hiding this comment

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

Looks good, fixes the bug, thanks!

dereksmart
dereksmart approved these changes Feb 10, 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 merged commit b0f6321 into master Feb 10, 2017
@dereksmart dereksmart deleted the fix/custom-share-options branch February 10, 2017 15:31
@dereksmart dereksmart added [Status] Needs Changelog [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Feb 10, 2017
jeherve added a commit that referenced this pull request Feb 21, 2017
@jeherve
Changelog: add #6355
c1dab1e
This was referenced Feb 22, 2017
Closed
Closed
dereksmart pushed a commit that referenced this pull request Feb 28, 2017
@jeherve @dereksmart
Jetpack 4.7: Changelog (#6331)
a9adf86 
* Changelog: update stable tag and move changelog to changelog.txt

Also remove old releases from readme.txt to keep the changelog tab short.

* Changelog: add #5883

Also update the filter's docblock to match new version.

* Changelog: add #5938

* Changelog: add #6298

* Changelog: add #3405

* Changelog: add #5941

* Changelog: add #6239

* Changelog: add #6281

* Changelog: add #6303

* Changelog: add #6018

* Changelog: add #6300

* Changelog: add #6296

* Changelog: add #6130

* Changelog: add #6292

* Readme: remove extra "on".

* Changelog: add #6307

* Changelog: add #3297

* Changelog: add #6275

* Changelog: add #6321

* Changelog: add #6297

* Readme: update the support forum link anchor.

Anchor changed when WordPress.org forums were updated to bbPress 2

* Readme: update list of a12s, it wasn't up to date anymore!

* Changelog: add #6338

* Changelog: add #6337

* Changelog: add #6335

* Changelog: add #6333

* Testing List: first version of the 4.7 testing list.

* Changelog: add #6332

* Changelog: add #6325

* Changelog: add #6326

* Changelog: add #6339

* Changelog: add #6342

* Changelog: add #6343

* Changelog: add #6346

* Changelog: add #6347

* Changelog: add #6279

* Changelog: add #6306

* Changelog: add #6312

* Changelog: add #6316

* Changelog: add #6171

* Changelog: add #6317

* Changelog: add #6246

* Changelog: add #6263

* Changelog: add #4220

* Changelog: add #5888

* Changelog: add #3406

* Changelog: add #3637

* Changelog: add #6320

* Changelog: add #5992

* Changelog: add #6322

* Changelog: add #6324

* Changelog: add #6352

* Changelog: add #6355

* Changelog: add #6360

* Changelog: add #6362

* Changelog: add #6369, #6382

* Changelog: add #6370

* Changelog: add #6375

* Changelog: add #6383

* Changelog: add #6384

* Changelog: add #6386

* Changelog: add #6395

* Changelog: add #6403

* Changelog: add #6406

* Changelog: add #6418

* Changelog: add #6419

* Changelog: add #6434

* Changelog: add #6446

* Changelog: add #6006

* Changelog: add #6096

* Changelog: add #6399

* Changelog: fix typo.

@see #6331 (comment)

* Changelog: add #6440

* Changelog: add #6443

* Changelog: add #6445

* Changelog: add #6463

* Changelog: add #6468

* Changelog: add #6471

* Changelog: add #6474

* Changelog: add #6480

* Changelog: add #6497

* Changelog: add #6499

* Changelog: add #6514

* Changelog: add #6267

* Changelog: add #5940

* Changelog: add #6492

* Changelog: add #5281

* Changelog: add #6327

* Changelog: add #6451

* Changelog: add #6525

* Changelog: add #6530
@jeherve jeherve mentioned this pull request Mar 6, 2017
Closed
zinigor added a commit that referenced this pull request Jul 26, 2017
@zinigor
Fixes default global options array merging.
26e4055 
A change introduced in #6355 started merging the regular options array with the global options. This has been largely unnoticed, but sometimes resulted in weird errors like this:

```
 PHP Catchable fatal error:  Argument 2 passed to Share_Twitter::__construct() must be of the type array, null given, called in /home/asotestjetpack/main/wp-content/plugins/jetpack-dev/modules/sharedaddy/sharing-service.php on line 227 and defined in /home/asotestjetpack/main/wp-content/plugins/jetpack-dev/modules/sharedaddy/sharing-sources.php on line 562
 PHP Warning:  array_merge(): Argument #1 is not an array in /home/asotestjetpack/main/wp-content/plugins/jetpack-dev/modules/sharedaddy/sharing-service.php on line 227
 PHP Notice:  Undefined index: global in /home/asotestjetpack/main/wp-content/plugins/jetpack-dev/modules/sharedaddy/sharing-service.php on line 196
```
@zinigor zinigor mentioned this pull request Jul 26, 2017
Merged
dereksmart pushed a commit that referenced this pull request Jul 31, 2017
@zinigor @dereksmart
Fixes default global options array merging. (#7538)
6fc40ed 
* Fixes default global options array merging.

A change introduced in #6355 started merging the regular options array with the global options. This has been largely unnoticed, but sometimes resulted in weird errors like this:

```
 PHP Catchable fatal error:  Argument 2 passed to Share_Twitter::__construct() must be of the type array, null given, called in /home/asotestjetpack/main/wp-content/plugins/jetpack-dev/modules/sharedaddy/sharing-service.php on line 227 and defined in /home/asotestjetpack/main/wp-content/plugins/jetpack-dev/modules/sharedaddy/sharing-sources.php on line 562
 PHP Warning:  array_merge(): Argument #1 is not an array in /home/asotestjetpack/main/wp-content/plugins/jetpack-dev/modules/sharedaddy/sharing-service.php on line 227
 PHP Notice:  Undefined index: global in /home/asotestjetpack/main/wp-content/plugins/jetpack-dev/modules/sharedaddy/sharing-service.php on line 196
```

* Improved array merge logic.

Props @eliorivero for the tip.
eliorivero pushed a commit that referenced this pull request Jul 31, 2017
@zinigor @eliorivero
Fixes default global options array merging. (#7538)
ccf4950 
* Fixes default global options array merging.

A change introduced in #6355 started merging the regular options array with the global options. This has been largely unnoticed, but sometimes resulted in weird errors like this:

```
 PHP Catchable fatal error:  Argument 2 passed to Share_Twitter::__construct() must be of the type array, null given, called in /home/asotestjetpack/main/wp-content/plugins/jetpack-dev/modules/sharedaddy/sharing-service.php on line 227 and defined in /home/asotestjetpack/main/wp-content/plugins/jetpack-dev/modules/sharedaddy/sharing-sources.php on line 562
 PHP Warning:  array_merge(): Argument #1 is not an array in /home/asotestjetpack/main/wp-content/plugins/jetpack-dev/modules/sharedaddy/sharing-service.php on line 227
 PHP Notice:  Undefined index: global in /home/asotestjetpack/main/wp-content/plugins/jetpack-dev/modules/sharedaddy/sharing-service.php on line 196
```

* Improved array merge logic.

Props @eliorivero for the tip.
@kraftbj kraftbj removed the [Status] Ready to Merge Go ahead, you can push that green button! label Oct 13, 2020
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

@jeherve jeherve Awaiting requested review from jeherve

+1 more reviewer

@zinigor zinigor zinigor approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@thingalon thingalon

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Sharing: can't add custom sharing services anymore

6 participants

@thingalon @zinigor @dereksmart @kraftbj @jeherve @matticbot