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.

Sign up for GitHub

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

Jump to bottom

Fix array->string conversion notice in carousel #8509

Merged
merged 1 commit into from
Jan 12, 2018
Merged

Fix array->string conversion notice in carousel #8509

merged 1 commit into from
Jan 12, 2018

Conversation

gravityrail
Copy link
Contributor

@gravityrail gravityrail commented Jan 11, 2018 •
edited by jeherve
Loading

Fixes an issue in Jetpack Carousel that printed an Array to string conversion notice on the screen.

Reported in https://wordpress.org/support/topic/php-notice-array-to-string-conversion-in-jetpack-carousel/ and 892311-zen.

To reproduce the issue:

  • add meta to an image, where that meta is an array
  • add that image to a gallery
  • output that gallery as a Jetpack carousel
  • you will see a notice like "Notice: Array to string conversion in
    /home/user/public_html/wp-content/plugins/jetpack/modules/carousel/jetpack-carousel.php on line 444

This PR removes non-scalar values from the array before applying strval.

@gravityrail gravityrail requested a review from a team as a code owner January 11, 2018 23:58
@gravityrail gravityrail added [Type] Bug When a feature is broken and / or not performing as intended [Feature] Carousel A fullscreen modal appearing when clicking on an image in a gallery or tiled gallery. [Status] Needs Review This PR is ready for review. labels Jan 11, 2018
@gravityrail gravityrail added this to the 5.8 milestone Jan 11, 2018
oskosk
oskosk approved these changes Jan 12, 2018
View reviewed changes
Copy link
Contributor

@oskosk oskosk left a comment

Choose a reason for hiding this comment

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

LGTM!

@oskosk oskosk 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 Jan 12, 2018
@dereksmart dereksmart merged commit e1e0f1d into master Jan 12, 2018
@dereksmart dereksmart deleted the fix/notice-on-array-image-meta branch January 12, 2018 21:08
@dereksmart dereksmart removed the [Status] Ready to Merge Go ahead, you can push that green button! label Jan 12, 2018
jeherve added a commit that referenced this pull request Jan 25, 2018
@jeherve
Changelog: add #8509
4d2e418
zinigor pushed a commit that referenced this pull request Jan 30, 2018
@oskosk @zinigor
Changelog 5.8: running changelog (#8442)
1ee2c03 
* Changelog 5.8: create base for changelog.

* Update 5.8 release post link

* fix 5.8 release date

* Updates to plugin description

* Changelog: add #8499

* Changelog: add #8506

* Changelog: add #8509

* Changelog: add #8516

* Changelog: add #8517

* Changelog: add #8523

* Changelog: add #8547

* Changelog: add #8496

* Changelog: add #8584

* Changelog: add #8595

* Changelog: add #8445

* Changelog: add #8431

* Changelog: add #8284

* Changelog: add #8270

* Changelog: add #8124

* Changelog: add #8581

* Changelog: add #8463

* Changelog: add #8568 (#8646)

* Updates to testing list and changelog

* Changelog: add #8443

* Changelog: add #8459

* Changelog: add #8469

* Changelog: add #8464

* Changelog: add #8478 and #8479

* Changelog: add #8483

* Changelog: add #8488

* Changelog: add #8513

* Changelog: add #8555

* Changelog: add #8565

* Changelog: add #8601

* Changelog: add #8612

* Changelog: add first pass at Search items.

* Changelog: add more info to help test Search.

* Changelog: add #8144

* Changelog: add #8313

* Changelog: add #8419

* Changelog: add #8465

* Changelog: add #8515

* Changelog: add #8587

* Changelog: add #8591

* Changelog: add #8659

* Changelog: add #8661

* Changelog: add #8671

* Changelog: add 5.7.1 to archived changelog too.

* Reverted changes to readme, removed entry about backups.
dereksmart pushed a commit that referenced this pull request May 2, 2018
@jhnstn @dereksmart
Carousel: merge changes from Jetpack
d9567d0 
Summary:
This merges this commit to the css file
 - adbde22

It also includes manual resolution for the divergent files:

- `mu-plugins/carousel/jetpack-carousel.php` -  significant commits:
   - #5469
   - 728c96f
   - 4b70301
   - 7fc7ebd
   - b4ad963
   - adbde22

- `mu-plugins/carousel/jetpack-carousel.js` - significant commits:
   - #8329
   - #7943
   - #8509

Test Plan:
Steps:

- Add an gallery widget to a site.
- Click on an image in the gallery
- The carousel should open and the navigation should work as expect.
- Check the dev console for any JS errors

Reviewers: dereksmart, zinigor

Reviewed By: zinigor

Subscribers: zinigor, brbrr, dereksmart

Differential Revision: https://[private link]

Merges r174217-wpcom.
dereksmart added a commit that referenced this pull request May 2, 2018
@dereksmart
Resolve divergence in file: whitespace (#9469)
962df70 
* Likes: update Carousel to remove Likes support, to match Jetpack.

Resolve divergences from Jetpack as well for related files.

Fixes #7133.

Merges r133276-wpcom.

* Carousel: better Jetpack JS hiding, see r133276.

Merges r133288-wpcom.

* Carousel: revert part of r133276 where galleries are auto closed.

Reported in Slack: https://a8c.slack.com/archives/triage/p1459368181000158

Jetpack related change: 36e529d

Merges r133533-wpcom.

* Jetpack: Sync carousel module changes

Summary: @michiecat mentioned today that a client was having an issue where opening a gallery, closing a gallery, and then clicking the back button was reopening the gallery. It looks like we have fixed this in Jetpack, but we need to sync those changes back to WPCOM so our WP.com and VIP users can get those changes.

Test Plan:
Check out patch. Sandbox site. Open gallery. Navigate through slides. Close gallery. Hit back button. Gallery should not open.

**Bonus feature**
Single images linked to the attachment URL should now open in a lightbox.  via #5469

Reviewers: samhotchkiss, dereksmart, eliorivero

Reviewed By: dereksmart, eliorivero

Differential Revision: https://[private link]

Merges r147170-wpcom.

* Carousel: merge changes from Jetpack

Summary:
This merges this commit to the css file
 - adbde22

It also includes manual resolution for the divergent files:

- `mu-plugins/carousel/jetpack-carousel.php` -  significant commits:
   - #5469
   - 728c96f
   - 4b70301
   - 7fc7ebd
   - b4ad963
   - adbde22

- `mu-plugins/carousel/jetpack-carousel.js` - significant commits:
   - #8329
   - #7943
   - #8509

Test Plan:
Steps:

- Add an gallery widget to a site.
- Click on an image in the gallery
- The carousel should open and the navigation should work as expect.
- Check the dev console for any JS errors

Reviewers: dereksmart, zinigor

Reviewed By: zinigor

Subscribers: zinigor, brbrr, dereksmart

Differential Revision: https://[private link]

Merges r174217-wpcom.

* Whitespace

* whitespace

* whitespace
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oskosk oskosk oskosk approved these changes

Assignees
No one assigned
Labels
[Feature] Carousel A fullscreen modal appearing when clicking on an image in a gallery or tiled gallery. [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Milestone
5.8
Development

Successfully merging this pull request may close these issues.
5 participants
@gravityrail @oskosk @jeherve @dereksmart @matticbot