Fork me on GitHub
Subscribe 3

Ticket #850 (fixed bug)

Avoid preg_replace() with PREG_REPLACE_EVAL

  • Created: 2013-04-10 16:37:24
  • Reported by: Franz
  • Assigned to: Franz
  • Milestone: 1.5.4
  • Component: code
  • Priority: normal

As mentioned here, the "e" modifier in regular expressions will be deprecated in PHP 5.5.

I didn't even know we were using such a feature...

History

Otomatic 2013-04-12 14:40:29

I think FluxBB use this "feature" because it is easier to use preg_replace mid modifier "e" than preg_replace_callback that does not accept array as callback function.

Personally, I prefer the alternative that performs only very few changes in the scripts where it will need to replace "preg_replace" with "preg_replace_callback," and only if you are using PHP 5.5.x.

I add two new functions in the file include/functions.php:

//[modif oto] for PHP 5.5.0 - Replacement of preg_replace and pattern modifier "e" with preg_replace_callback
function php55_callback($element)
{
	return str_replace(
		array('\'$1$3\'','\'$2$4\'','\'$1\'', '\'$2\'', '$1', '$2'),
		array('$matches[1].$matches[3]','$matches[2].$matches[4]','$matches[1]','$matches[2]','$matches[1]','$matches[2]'),
		$element);
}

//Separate patterns with "e" modifier and without
function php55_preg_replace($pattern, $replace, $text)
{
	//if not PHP 5.5.x no changes
	if(version_compare(PHP_VERSION, '5.5.0', '<'))
		return preg_replace($pattern, $replace, $text);

	if(!is_array($pattern)) {
		$pattern = array($pattern);
		$replace = array($replace);
	}
	$count = count($pattern);
	$pattern_normal = $replace_normal = array();
	for($i = 0; $i < $count ; $i++)
	{
		if(substr($pattern[$i],-1) == "e")
			$text = preg_replace_callback(substr($pattern[$i],0,-1), create_function('$matches','return '.php55_callback($replace[$i]).';'), $text);
		else
		{
			$pattern_normal[] = $pattern[$i];
			$replace_normal[] = $replace[$i];
		}
	}
	if(empty($pattern_normal))
		return $text;
	else
		return preg_replace($pattern_normal, $replace_normal, $text);
}

Then it will only be necessary to change only the calls "preg_replace" who need it to "php55_preg_replace."

Comment edited 2 times (Diff, Diff 2)

Otomatic 2013-04-15 16:14:49

Hi,

If we do not add new function php55_preg_replace, in parser.php file, there are about six lines with preg_replace to be changed into preg_replace_callback and about twelve array indexes $pattern and $replace to be changed into $callback_pattern and $callback_replace plus a loop to treat them.

What do you recommend?

Franz 2013-04-15 20:46:08

Would these lines get much longer? I generally prefer changing the calls to preg_replace_callback() directly without the somewhat "magic" function.

Otomatic 2013-04-16 14:11:33

Hi,
It is also my opinion. It is preferable to use the appropriate functions, especially as it is backward compatible with earlier versions of PHP.
After research, only the file parser.php and the function "ucp_preg_replace" in the functions.php file must be modified.
How can I upload the two modified files?

Otomatic 2013-04-18 08:25:32

  • Uploaded patch parser.diff. (view)

Hi,

Function ucp_preg_replace modified:

//
// Replace string matching regular expression
//
// This function takes care of possibly disabled unicode properties in PCRE builds
// [modif oto] - Added $callback parameter to use preg_replace_callback
// in place of preg_replace with modifier "e" on pattern
function ucp_preg_replace($pattern, $replace, $subject, $callback = false)
{
	if($callback) 
		$replaced = preg_replace_callback($pattern, create_function('$matches', 'return '.$replace.';'), $subject);
	else
		$replaced = preg_replace($pattern, $replace, $subject);

	// If preg_replace() returns false, this probably means unicode support is not built-in, so we need to modify the pattern a little
	if ($replaced === false)
	{
		if (is_array($pattern))
		{
			foreach ($pattern as $cur_key => $cur_pattern)
				$pattern[$cur_key] = str_replace('\p{L}\p{N}', '\w', $cur_pattern);

			$replaced = preg_replace($pattern, $replace, $subject);
		}
		else
			$replaced = preg_replace(str_replace('\p{L}\p{N}', '\w', $pattern), $replace, $subject);
	}

	return $replaced;
}

And a .diff file for parser.php. I generate this file whith winmerge on Windows 7 Pro 64 bits.
I hope it is correct.

Comment edited 1 times (Diff)

Franz 2013-04-18 08:44:02

  • Owner set to Franz.

Thanks, Oto. Seems good, I'll take a look soon.

adaur 2013-04-18 16:06:34

Maybe I'm doing this wrong, but when I try to post "www.test.com" with Oto's parser, I'm getting this message:

[url=http://wwwtest.com]wwwtest.com[/url]

instead of

[url=http://www.test.com]www.test.com[/url]
Comment edited 1 times (Diff)

Otomatic 2013-04-18 17:08:01

I think I forgot a "dot" when I tranform replace string for regex into function.
To help me, can you explain what you do to obtain this mistake.

adaur 2013-04-18 17:14:55

As I said: I applied your patch above and modified my functions.php, and tried to post the following message:

www.test.com

FluxBB returns:

[url=http://wwwtest.com]wwwtest.com[/url]

www is taken as a part of the website URL

Otomatic 2013-04-19 08:05:53

Hi,
Missing a dot (twice) in the second call to "ucp_preg_replace" in the "do_clickable" function.
In the function "do_clickable" replace the second call to ucp_preg_replace that is :

$text = ucp_preg_replace('%(?<=[\s\]\)])(<)?(\[)?(\()?([\'"]?)(www|ftp)\.(([\p{L}\p{N}\-]+\.)+[\p{L}\p{N}]+(:[0-9]+)?(/(?:[^\s\[]*[^\s.,?!\[;:-])?)?)\4(?(3)(\)))(?(2)(\]))(?(1)(>))(?![^\s]*\[/(?:url|img)\])%ui','stripslashes($matches[1].$matches[2].$matches[3].$matches[4]).handle_url_tag($matches[5].$matches[6], $matches[5].$matches[6], true).stripslashes($matches[4].$matches[10].$matches[11].$matches[12])', $text, true);

by:

$text = ucp_preg_replace('%(?<=[\s\]\)])(<)?(\[)?(\()?([\'"]?)(www|ftp)\.(([\p{L}\p{N}\-]+\.)+[\p{L}\p{N}]+(:[0-9]+)?(/(?:[^\s\[]*[^\s.,?!\[;:-])?)?)\4(?(3)(\)))(?(2)(\]))(?(1)(>))(?![^\s]*\[/(?:url|img)\])%ui','stripslashes($matches[1].$matches[2].$matches[3].$matches[4]).handle_url_tag($matches[5].".".$matches[6], $matches[5].".".$matches[6], true).stripslashes($matches[4].$matches[10].$matches[11].$matches[12])', $text, true);

Should I create a new "diff" file?

Nota : $5$6 is not the same as $5.$6 in regex!

Comment edited 1 times (Diff)

adaur 2013-04-19 16:37:15

Commit 404e5c2 to fluxbb master

#850: Avoid preg_replace() with PREG_REPLACE_EVAL

All credit goes to Otomatic

adaur 2013-04-19 16:38:57

It's working now. I have submitted a pull request on GitHub, I hope you don't mind.

adaur 2013-04-19 16:39:28

Commit dfea314 to fluxbb master

#850: updating ucp_preg_replace function

All credit goes to Otomatic

Franz 2013-04-19 19:37:17

Commit 85ea7f4 to fluxbb master

Merge pull request #74 from adaur/master

#850: Avoid preg_replace() with PREG_REPLACE_EVAL

adaur 2013-04-19 19:50:48

  • Status changed from open to fixed.