Skip to content

Conversation

@nathkrill
Copy link

Use preg_last_error() inside convert_smilies() to check for any preg_split errors before running count().
If there are errors, sends a PHP warning with the error message, then returns the original text.

Running count() on the array after there are preg_split errors can cause post content to not display (see ticket).

This is a reformat of a previous pull request to get the PHP tests to pass.

Trac ticket: https://core.trac.wordpress.org/ticket/51019


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

@petri8
Copy link

petri8 commented Mar 11, 2022

I am having the same issue. I have multiple images with base64 as url. While loading the page the images are missing because preg_split() can't handle the bigger size of the string and thus returns false. Please add a check for this function and merge this pull request. Thank you!

@alex-lucaci
Copy link

Yeah, I've also seen the issue reported by @petri8 . It happens when the $text is too large and $textarr becomes false.

I believe it's an easy fix.

@borzaszto
Copy link

Same, +1 Up 🙌

@lzudor-omnitech
Copy link

Is there any ETA on this..?

$textarr = preg_split( '/(<.*>)/U', $text, -1, PREG_SPLIT_DELIM_CAPTURE ); // Capture the tags as well as in between.

if ( preg_last_error() != PREG_NO_ERROR ) {
trigger_error( preg_last_error_msg(), E_USER_WARNING );
Copy link
Member

Choose a reason for hiding this comment

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

Instead of using trigger_error, wp_trigger_error should be used. First pass at a more informative error message.

Suggested change
trigger_error( preg_last_error_msg(), E_USER_WARNING );
$message = sprintf( '<code>convert_smilies</code> could not split text. Error Message: %s', preg_last_error_message() );
wp_trigger_error( 'convert_smilies', $message, E_USER_WARNING );

@audrasjb
Copy link
Contributor

@nathkrill are you able to refresh your PR with the requested changes? Thank you!

@github-actions
Copy link

github-actions bot commented Feb 19, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Unlinked Accounts

The following contributors have not linked their GitHub and WordPress.org accounts: @nathancrowd, @nathkrill, @petri8, @alex-lucaci, @borzaszto, @lzudor-omnitech.

Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases.

Core Committers: Use this line as a base for the props when committing in SVN:

Props jorbin, audrasjb.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@lzudor-omnitech
Copy link

Is this issue abandoned now..?

// HTML loop taken from texturize function, could possible be consolidated.
$textarr = preg_split( '/(<.*>)/U', $text, -1, PREG_SPLIT_DELIM_CAPTURE ); // Capture the tags as well as in between.

if ( preg_last_error() != PREG_NO_ERROR ) {
Copy link

Choose a reason for hiding this comment

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

Suggested change
if ( preg_last_error() != PREG_NO_ERROR ) {
if ( preg_last_error() !== PREG_NO_ERROR ) {

Should we use strict checking here? Note that PREG_NO_ERROR is 0.

@SergeyBiryukov
Copy link
Member

Thanks for the PR! Merged a slightly different approach in r59515.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants