PHP7 and the preg_replace problem

[gnif]

Changing code and making it work for one single installation of phpBB is one thing, but when you take into consideration that there are thousands of installations of phpBB 3.1.x with varying board and server configurations, all using hundreds or even thousands of different custom BBCodes, and that the current release is stable for all of those it would be irresponsible of us to suddenly make a change that would affect 100% of all updated installations without knowing if those installations would still work or not or if there will be data loss.

I completely agree with and understand this, except that preg_replace_callback has existed since PHP 4.0.5 which was released 15 years ago, and phpBB3 was released 9 years ago and has gone though multiple versions since, and has ignored all the warnings from PHP 5.5 (3 years ago) onwards that the 'e' modifier is deprecated and should not be used. I stand by my comment that it is abysmal that it has not yet been updated.

Edit: The 'e' modifier has been known for over 10 years now to be dangerous and has been abused time and time again to break into websites through unexpected bugs. It's removal has been planned from the date that preg_replace_callback was added to PHP due to the security implications, and when PHP5 added Closures (anonymous functions) there was no longer any reason to retain the 'e' modifier. Every programmer on the planet should cringe at the idea of methods that parse strings as code, methods such as 'eval' should have never existed in the first place for security, performance and code maintance reasons.

[DavidIQ]

I completely agree with and understand this, except that preg_replace_callback has existed since PHP 4.0.5 which was released 15 years ago, and phpBB3 was released 9 years ago and has gone though multiple versions since, and has ignored all the warnings from PHP 5.5 (3 years ago) onwards that the 'e' modifier is deprecated and should not be used. I stand by my comment that it is abysmal that it has not yet been updated.

Edit: The 'e' modifier has been known for over 10 years now to be dangerous and has been abused time and time again to break into websites through unexpected bugs. It's removal has been planned from the date that preg_replace_callback was added to PHP due to the security implications, and when PHP5 added Closures (anonymous functions) there was no longer any reason to retain the 'e' modifier. Every programmer on the planet should cringe at the idea of methods that parse strings as code, methods such as 'eval' should have never existed in the first place for security, performance and code maintance reasons.

I don't disagree with the fact that it should have been taken care of at some point, but they weren't warnings that were just ignored. I think that, at least in part, warnings about this not coming up in phpBB installations (albeit probably due to some code we've added) and the use of the e modifier with preg_replace not causing any security issues in the lifetime of phpBB 3.x are reasons that likely contributed to just leaving the BBCode parser alone for so long, not because we've been ignoring the warnings.

[gnif]

I completely agree with and understand this, except that preg_replace_callback has existed since PHP 4.0.5 which was released 15 years ago, and phpBB3 was released 9 years ago and has gone though multiple versions since, and has ignored all the warnings from PHP 5.5 (3 years ago) onwards that the 'e' modifier is deprecated and should not be used. I stand by my comment that it is abysmal that it has not yet been updated.

Edit: The 'e' modifier has been known for over 10 years now to be dangerous and has been abused time and time again to break into websites through unexpected bugs. It's removal has been planned from the date that preg_replace_callback was added to PHP due to the security implications, and when PHP5 added Closures (anonymous functions) there was no longer any reason to retain the 'e' modifier. Every programmer on the planet should cringe at the idea of methods that parse strings as code, methods such as 'eval' should have never existed in the first place for security, performance and code maintance reasons.

I don't disagree with the fact that it should have been taken care of at some point, but they weren't warnings that were just ignored. I think that, at least in part, warnings about this not coming up in phpBB installations (albeit probably due to some code we've added) and the use of the e modifier with preg_replace not causing any security issues in the lifetime of phpBB 3.x are reasons that likely contributed to just leaving the BBCode parser alone for so long, not because we've been ignoring the warnings.

Either way, this is something that should have been fixed many years ago, deprecation warnings should be taken seriously. Perhaps they were noted, but they should have been given the highest of priorities to fix it, or atleast inform the general public of the need to stop using 'e' modifiers in bbcodes. Bakwards compatibible code could have been added that forum admins could opt into should the new code break mods/extensions. It would have provided a way for hosts to provide later PHP version options to those that requre it, or even encouraged mod/extension developers to update their code for the better performing callback method.

I agree that moving forward with platform breaking code is not acceptible, but in this instance the code that was written and released by phpBB 9 years ago was already outdated. Ignored deprecation warnings just hurts your project and turns people away from it.

For example, my client was ready to move away from phpBB3 due to its performance, even though their forum is one of the largest I have ever encountered. The quickest way to extract higher performance from it was to move to PHP7, but obvioulsy this did not work. Their reaction to the error when they saw it was that phpBB do not keep their code current, or atleast provide a means to be forward compatible.

Moving to phpBB3.3 is never likely to occur on this platform since you have redeveloped the bbcode stuff completely, the forum is too massive and too many clients to risk on a complete rewrite of code that should have just been patched as a point release.

Edit: I just re read your post sorry, "albeit probably due to some code we've added". Quite likely the case as deprecation warnings can be disabled and is often done, but if you are in a dev environment ALL warnings should be enabled to ensure your code completely complies with standards.

[DavidIQ]

deprecation warnings should be taken seriously. Perhaps they were noted, but they should have been given the highest of priorities to fix it, or atleast inform the general public of the need to stop using 'e' modifiers in bbcodes.

They would have been given high priority had they come up, which is the point. They never came up after the release of Olympus. The dev team today is entirely different from the Development Team that started phpBB3 so maybe this was something that a developer "took care of" by changing the PHP warning levels for phpBB and meant to come back to it later and really take care of it, but rode off into the sunset instead. Bad practice? Probably, but the Development Team has made many changes to try and make sure things like that don't happen anymore (this probably highlights the importance of code reviews, which didn't really exist when we were using svn back in the days of Olympus development).

Edit:

Edit: I just re read your post sorry, "albeit probably due to some code we've added". Quite likely the case as deprecation warnings can be disabled and is often done, but if you are in a dev environment ALL warnings should be enabled to ensure your code completely complies with standards.

Yes, definitely

[Marc]

We welcome any input regarding the development process or ideas on new features, changes, etc. Back when phpBB 3.0 was released, this obviously was not outdated (latest PHP version was 5.2). Using the /e modifer was a perfectly valid use in preg_replace(). As PHP was developed further, it was decided to deprecate the modifier in PHP 5.5, which was released in 2013. From the code changes I can see that the deprecation notice was disabled to not prevent this from appearing in production use. Switching to preg_replace_callback() as soon as it was added wouldn't have been a viable move due to the issues described with custom BBCodes. A simple drop-in is not easily done. Also, quickly moving over to newly added functions is not always the best viable option.
I'd very much welcome your input on how to get rid of it for phpBB 3.1 in order to be able to see if this is something that could be added in a maintenance release of phpBB 3.1.

As you might have noticed, everyone working on phpBB is a volunteer. That means we don't get paid to work on phpBB and do this in our free time. Initial ideas to replace our BBCode engine started in 2010 and were further detailed in 2013 when JoshyPHP stepped up to help with this. At the point in time it was finished and considered stable enough to be merged into phpBB, the feature freeze in phpBB 3.1 had already been reached. That's also the reason why it didn't make it into phpBB 3.1.
This is not something that can be finished in a few weeks like it might be with people working full time on phpBB.

Please note that switching from the old BBCode engine to the new one is not a breaking change. Posts, topics, etc. will simply be reparsed into the new format.
I'd also like to note that phpBB outperforms for example a simple Wordpress site. This does get even better when using opcache and faster memory caching.

[gnif]

it was decided to deprecate the modifier in PHP 5.5

It was deprecated in PHP5.4 (2012), but I see your point.

I'd very much welcome your input on how to get rid of it for phpBB 3.1 in order to be able to see if this is something that could be added in a maintenance release of phpBB 3.1.

I will see if I can find some time in the coming week to contribute this and a few other fixes.

As you might have noticed, everyone working on phpBB is a volunteer. That means we don't get paid to work on phpBB and do this in our free time. Initial ideas to replace our BBCode engine started in 2010 and were further detailed in 2013 when JoshyPHP stepped up to help with this. At the point in time it was finished and considered stable enough to be merged into phpBB, the feature freeze in phpBB 3.1 had already been reached. That's also the reason why it didn't make it into phpBB 3.1.
This is not something that can be finished in a few weeks like it might be with people working full time on phpBB.

I appreciate this and the hard work that goes into such a project, I am also a major contributer to open source projects (Kodi primarily, C++ but my first love will always be PHP) and know where you are coming from first hand.

Please note that switching from the old BBCode engine to the new one is not a breaking change. Posts, topics, etc. will simply be reparsed into the new format.
I'd also like to note that phpBB outperforms for example a simple Wordpress site. This does get even better when using opcache and faster memory caching.

Noted, it does perform well but in comparison to some of the leaner boards such as SMF it does not do so well. There are quite a few areas phpBB could be improved such as removal of the use of 'compact' and 'extract' which prevent jit from being able to predict branches, this is the main reason why running phpBB under something like HHVM does not seem to give much of a performance boost.

[AmigoJack]

There are quite a few areas phpBB could be improved such as removal of the use of 'compact' and 'extract' which prevent jit from being able to predict branches, this is the main reason why running phpBB under something like HHVM does not seem to give much of a performance boost.

Surprise, surprise:

1. The trigger check is so not being optimized for performance:

   Code: Select all

   extract($phpbb_dispatcher->trigger_event('core.viewforum_get_topic_data', compact($vars)));

   This call is sub-optimal, and I don't know why this call pattern has evolved. Cons:

   1. extract() is slower than i.e. foreach() $$var= $value
   2. extract() might fail when incorrect data is passed. Documentation comments say it fails for everything, including data which might be processed/parsed correctly.
   3. compact() creates variable copies - why not directly using references? That would also kill the need of extract()
   4. Both functions are always called (including all the work they do), even if they're not needed at all (no event matches to be triggered). Why not having an implementation where an if first checks for trigger matches and only after that all the extract/compact work is done? Considering how many times event trigger checks are encountered (and will surely increase in future releases) this is a questionable waste of overhead.

[gnif]

1. extract() is slower than i.e. foreach() $$var= $value

Dynamic variable access should also be avoided, again due to inability to perform branch prediction by the optomizer.

See HHVM Optimization Tips. This applies to PHP7 also as it also does JIT compilation.

[kangus]

I will see if I can find some time in the coming week to contribute this and a few other fixes.

This is 8/21/2016 or 4 months later, I downloaded the latest 3.1x and the problem is still there.

Found out this morning that:
1. Ubuntu installs php7 by default
2. 3.2 is the only version that supports php7 but it is beta and it will be months if not years before the Styles, Mods and other infrastructures catch up.

[DavidIQ]

2. 3.2 is the only version that supports php7 but it is beta and it will be months if not years before the Styles, Mods and other infrastructures catch up.

3.2 is in Release Candidate stage, not beta. Some Extensions (MODs are already a thing of the past) might need some work to be 3.2 compatible but not a whole lot. We don't expect the adjustments of extensions to be too complex for the most part to get them working in 3.2.

[beatme101]

As an experiment we decided to trial PHP7 on one of the servers and saw a large performance improvement, but obviously the preg_replace issue everyhone else here has reported. We spent the next day patching every occurance of preg_replace with the 'e' modifier to use preg_replace_callback with Closure methods, retaining 100% compatibility with the existing code, which gave us a fully working phpBB3 forum on PHP7.

Can I get a little more information on this, like perhaps code? I'm trying to make the same changes to my own forum as I don't want to have to disable bbcode now that everything is on PHP 7. I don't use any custom bbcode so that shouldn't be a problem either.

Edit: just going to mention that this thread is one of the very few results on google for the error message the problem causes.

[AmigoJack]

Example:

Code: Select all

preg_replace
( '/foo/e'
, "\$user-> lang['\$1'])"
, $in 
) 

would become

Code: Select all

preg_replace_callback
( '/foo/'
, function( $match ) use( $user ) { return $user-> lang[$match[1]]; }
, $in 
) 

...so it's not that difficult.
As for /includes/bbcode.php 3.2.0-RC1 solves this even more easily:

Code: Select all

$message = preg_replace($preg['search'], $preg['replace'], $message);

became (slightly altered)

Code: Select all

if (is_callable($preg['replace'])) {
    $message = preg_replace_callback($preg['search'], $preg['replace'], $message);
} else {
    $message = preg_replace($preg['search'], $preg['replace'], $message);
}

Read http://php.net/manual/en/class.closure.php and/or http://php.net/manual/en/function.preg- ... php#109938.

[beatme101]

Unfortunately that's giving the same error. And while that code is sort of readable at a glance, line 494 isn't, and I'm getting alot more errors from it:

Code: Select all

$tpl = preg_replace('/{L_([A-Z0-9_]+)}/e', "(!empty(\$user->lang['\$1'])) ? \$user->lang['\$1'] : ucwords(strtolower(str_replace('_', ' ', '\$1')))", $tpl);

This stuff is a bit beyond what I've worked on in the past.

[AmigoJack]

the same error
alot more errors

1. Make sure you're actually reading the error messages to get a clue on the cause - just counting them is no help for anyone.
2. Make sure you're distinguishing between errors, warnings and hints.
3. Your example took me seconds to adapt and should work, however I haven't tested it:

Code: Select all

$tpl = preg_replace_callback('/{L_([A-Z0-9_]+)}/', function( $match ) use( $user ) { return (!empty($user->lang[$match[1]])) ? $user->lang[$match[1]] : ucwords(strtolower(str_replace('_', ' ', $match[1]))); }, $tpl); 

Please keep in mind that this board is not for helping with PHP in general - I already linked to the manual.

[beatme101]

1. Make sure you're actually reading the error messages to get a clue on the cause - just counting them is no help for anyone.

http://hurr.org/i/phpbb_preg_replace_callback.png

[phpBB Debug] PHP Warning: in file [ROOT]/includes/bbcode.php on line 494: preg_replace(): The /e modifier is no longer supported, use preg_replace_callback instead
[phpBB Debug] PHP Warning: in file [ROOT]/includes/bbcode.php on line 113: preg_replace(): The /e modifier is no longer supported, use preg_replace_callback instead

When I used the 5 line replacement you posted before, the 113 error moved down to the new line preg_replace was on. if you had php 7, and tried that code, you'd see the same error. I can read error messages, obviously, but I don't understand this code.

The line 494 replacement works.

I'd be happy to create a support thread but this seemed like an appropriate thread to have a solution posted, I assumed someone could fit it in a single post.