Skip to content

Responsive Videos: avoid notice when param is not given.#10109

Merged
oskosk merged 1 commit intomasterfrom
fix/missing-default-resp-3048
Sep 14, 2018
Merged

Responsive Videos: avoid notice when param is not given.#10109
oskosk merged 1 commit intomasterfrom
fix/missing-default-resp-3048

Conversation

@jeherve
Copy link
Copy Markdown
Member

@jeherve jeherve commented Sep 6, 2018

Fixes #3048

Changes proposed in this Pull Request:

Avoid PHP notice when param is not given.

This was already fixed in #3243, but got reverted by mistake in ab7f2e0

Testing instructions:

  1. Add the following to your theme's functions.php file or to a functionality plugin:
function jeherve_enable_resp() {
       	add_theme_support( 'jetpack-responsive-videos' );
}
add_action( 'after_setup_theme', 'jeherve_enable_resp' );
  1. Add a few videos from YouTube or Vimeo to your posts.
  2. View your site on a mobile device.
  3. Make sure you do not see any PHP notices.

Proposed changelog entry for your changes:

Responsive Videos: avoid PHP notice.

@jeherve
Responsive Videos: avoid notice when param is not given.
1251ac1 
Fixes #3048

This was already fixed in #3243, but got reverted by mistake in ab7f2e0
@jeherve jeherve added Bug When a feature is broken and / or not performing as intended [Feature] Theme Tools [Status] Needs Review This PR is ready for review. labels Sep 6, 2018
@jeherve jeherve added this to the 6.6 milestone Sep 6, 2018
@jeherve jeherve self-assigned this Sep 6, 2018
@jeherve jeherve requested a review from a team as a code owner September 6, 2018 14:40
@matticbot
Copy link
Copy Markdown
Contributor

matticbot commented Sep 6, 2018

Caution: This PR has changes that must be merged to WordPress.com
Please review this diff before merging: D17995-code. (newly created revision)

@jetpackbot
Copy link
Copy Markdown
Collaborator

jetpackbot commented Sep 7, 2018

That's a great PR description, thank you so much for your effort!

Generated by 🚫 dangerJS

@mdawaffe
Copy link
Copy Markdown
Member

mdawaffe commented Sep 7, 2018

I don't see any PHP Notices even without the patch. What am I doing wrong?

Theme: 2017

@jeherve
Copy link
Copy Markdown
Member Author

jeherve commented Sep 7, 2018

It seems to only happen with some specific themes. The last user report was from a user using a Premium theme, so I could not download it to test either. :(

I assume that some themes call the jetpack_responsive_videos_maybe_wrap_oembed function directly without passing it that second parameter.

@oskosk
Copy link
Copy Markdown
Contributor

oskosk commented Sep 11, 2018

I can't reproduce the warning with the provided steps :( Found that a theme like slik-lite should be useful for testing cause there are a few sites in the wild using this theme and showing the warning right now.

@jeherve
Copy link
Copy Markdown
Member Author

jeherve commented Sep 12, 2018

It looks like that theme includes a full copy fo the Responsive Videos tool:
https://themes.svn.wordpress.org/silk-lite/1.2.8/inc/jetpack/responsive-videos.php

@brbrr
Copy link
Copy Markdown
Contributor

brbrr commented Sep 14, 2018

I wasn't able to reproduce it either :(

@oskosk
Copy link
Copy Markdown
Contributor

oskosk commented Sep 14, 2018

Just realized that this PR is introducing something that was already there. I'm merging it.

@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 Sep 14, 2018
oskosk
oskosk approved these changes Sep 14, 2018
View reviewed changes
Copy link
Copy Markdown
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 given that this piece of code was already supposed to be like this

@oskosk oskosk merged commit 864587b into master Sep 14, 2018
@oskosk oskosk deleted the fix/missing-default-resp-3048 branch September 14, 2018 17:02
@ghost ghost removed the [Status] Ready to Merge Go ahead, you can push that green button! label Sep 14, 2018
jeherve added a commit that referenced this pull request Sep 24, 2018
@jeherve
Changelog: add #10109
7b5bfc6
jeherve added a commit that referenced this pull request Sep 25, 2018
@jeherve
Changelog for 6.6 release (#10107)
581489a 
* Readme: add boilerplate for next release, 6.6

* Add 6.5 to the changelog.txt file

* Set boilerplate testing list for 6.6

* Readme: update stable tag to 6.5

* Add bullets to 6.5 changelog items

* Readme: add link to previous changelogs

This will help folks who want to know more about past releases,
while keeping the readme.txt short so as to not overwhelm translators and site owners only looking for information about the last release.

* Changelog: add information at the top of the changelog file.

* Changelog: add #10054

* Changelog: add #10078

* Changelog: add #10079

* Changelog: add #10064

* Changelog: add #10094

* Changelog: add #10096

* Testing list: add more information based on #10087

* Changelog: add #9847

* Changelog: add #10084

* Changelog: add #9918

* Changelog: add #7614

* Changelog: add #10116

* Changelog: add #10108

* Changelog: add #10041

* Changelog: add #10121

* Changelog: add #10134

* Changelog: add #10130

* Changelog: add #10109

* changelog: add #10137

* changelog: add #9952

* changelog: add #10120

* changelog: add #10162

* Changelog: add #10163

* Changelog: add #10092

* changelog: add #10156

* Changelog: add #10154

* changelog: add #10122

* Changelog: add #10101

* changelog: add #10105

* changelog: add #10190

* Changelog: add #10196

* changelog: add #10152

* Changelog: add #10153

* Testing list: add more details to Site Verification testing steps.

@see #10143 (comment)

* changelog: add #10194

* Changelog: add #10193
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@oskosk oskosk oskosk approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@jeherve jeherve

Labels

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

Projects

None yet

Milestone

6.6

Development

Successfully merging this pull request may close these issues.

Responsive videos display Warning

6 participants

@jeherve @matticbot @jetpackbot @mdawaffe @oskosk @brbrr