Skip to content

fix PHP warnings about non-string#6157

Closed
cliffordp wants to merge 2 commits intoAutomattic:masterfrom
cliffordp:patch-1
Closed

fix PHP warnings about non-string#6157
cliffordp wants to merge 2 commits intoAutomattic:masterfrom
cliffordp:patch-1

Conversation

@cliffordp
Copy link
Copy Markdown

Stops the generation of these WP_DEBUG messages (https://gist.github.com/cliffordp/42b277d0321c78f59cd2d57faeb0a93d) with the Community Events plugin by Modern Tribe (https://theeventscalendar.com/product/wordpress-community-events/) when Shortcode Embeds is active (https://central.tri.be/issues/72412) and Community Events reCAPTCHA is in use (e.g. anonymous submissions)

Fixes #

Changes proposed in this Pull Request:

Testing instructions:

Proposed changelog entry for your changes:


Stops the generation of these WP_DEBUG messages (https://gist.github.com/cliffordp/42b277d0321c78f59cd2d57faeb0a93d) with the Community Events plugin by Modern Tribe (https://theeventscalendar.com/product/wordpress-community-events/) when Shortcode Embeds is active (https://central.tri.be/issues/72412) and Community Events reCAPTCHA is in use (e.g. anonymous submissions)
@jeherve jeherve added [Feature] Shortcodes / Embeds [Pri] High [Status] Needs Review This PR is ready for review. Bug When a feature is broken and / or not performing as intended labels Jan 25, 2017
@jeherve jeherve added this to the 4.5.1 milestone Jan 25, 2017
Copy link
Copy Markdown
Contributor

@georgestephanis georgestephanis left a comment

Choose a reason for hiding this comment

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

I'm somewhat careful on just short circuiting when input isn't what is expected, fearing we may let something concerning through (at first glance anyways) -- can you give a quick example for why $html wouldn't be a string in this instance, and what we can do to more safely handle that use case?

foreach ( $regexps as $element => $regexp ) {
self::$current_element = $element;

if ( ! is_string( $html ) || ! is_string( $element ) ) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think it's necessary to include the second is_string() here -- $element is the key from the array, so there shouldn't be any issues with it.

@jeherve jeherve added [Status] In Progress [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Needs Review This PR is ready for review. labels Jan 26, 2017
@cliffordp
Copy link
Copy Markdown
Author

@georgestephanis, I added

var_dump($html);
var_dump($element);

and got a lot of these:

  • image
  • image
  • image
  • and many more

So it looks like you could drop || ! is_string( $element )

I'm not really sure where these empty arrays for $html are coming from.

Here's a copy of the latest version of Community Events (CE) add-on for https://wordpress.org/plugins/the-events-calendar/ --
the-events-calendar-community-events.4.4.1.zip

Here's how to reproduce the issue:

  • Enable Jetpack 4.5's Shortcode Embeds: https://cl.ly/1p3S2a3o3q02
  • Make sure reCAPTCHA and Anonymous submissions are enabled
  • Try to create a new event via CE form (error happens when you click Submit)

@jeherve jeherve added [Status] Needs Review This PR is ready for review. and removed [Status] In Progress [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Jan 27, 2017
@jeherve jeherve modified the milestones: 4.5.1, 2/17 - February Jan 30, 2017
@georgestephanis
Copy link
Copy Markdown
Contributor

I'm talking to @cliffordp in .org Slack right now -- this isn't a Jetpack problem, it looks like it's a case of another plugin passing an array instead of a string to wp_kses_post()

Closing as it's not ours to fix.

@cliffordp cliffordp deleted the patch-1 branch February 1, 2017 01:49
@cliffordp
Copy link
Copy Markdown
Author

Thanks so much for your help, @georgestephanis!!!

@jeherve jeherve removed the [Status] Needs Review This PR is ready for review. label Feb 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug When a feature is broken and / or not performing as intended [Feature] Shortcodes / Embeds [Pri] High

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants