Require PCRE Version >= 7.2

The same issue with the same error message on screen.
Drupal 7.x-beta2

I think this is the same issue I just ran across. Here's the error message I recevied when I tried to turn on CSS aggregation:

Warning: preg_replace(): Compilation failed: unrecognized character after (? at offset 9 in drupal_load_stylesheet_content() (line 3504 of /var/www/drupal-cvs/includes/common.inc).

I'm tempted to bump the priority to critical since this totally broke my development site when I turned this on. But I'll check to see if this was reported elsewhere first. This is seems so obvious that i suspect that this bug report may be duplicative of another one.

P.S.: This issue did not come up when I turned on javascript aggregation.

anyone else seeing this? I've seen some bugs that seem to be related to this but none with the detailed error message here. Even though I think this bug should go to critical right now because normal settings for optimizing a drupal site leads to a show-stopping bug. But I'll only put to major so that it gets more attention.

Please vote this back down to major if this is an edge case. I don't see another bug talking about this issue, but it occurs for me everytime. I just want to make sure this isn't a widespread issue. And it does break CSS aggregation so I seems like a critical bug to me.

I wonder if this is a bug in PCRE -- what version does phpinfo() tell you that you have? If you just copy that preg_replace call to a file, can you run it?

Your right, the version of pcre my phpinfo() reports is 6.6 06-Feb-2006. Yep, 2006! http://www.pcre.org/ reports that the latest version of pcre is 8.10. Tonight, I'll attempt to upgrade and see if the issue persists.

If this upgrade turns out to resolve this issue, I recommend that we make sure that in our documentation for drupal 7 we suggest a minimum version for pcre in our requirements.

The changelog for PCRE is quite lengthy -- it might be hard to figure out a minimum version. For what it's worth, I've got version 8.0 and it works fine.

http://www.linux-archive.org/centos/118621-utf-8-support-pcre-2.html

Looks a lot like the issue I'm having and yes my pcre doesn't have --enable-unicode-properties turned on.

Found it...

We need to add to the list of requirements a version of PCRE which is at least "Version 7.2 19-Jun-07":

From the PCRE changelog:

Version 7.2 19-Jun-07
---------------------
6. Added more features from the forthcoming Perl 5.10:

(g) (?| introduces a group in which the numbering of parentheses in each alternative starts with the same number.

This (?| construction is used in drupal_load_stylesheet_content, for example.

I run CentOS 5.2, which seems to be hard set on using the version of PCRE that I'm currently using. I run a dedicated virtual machine at MediaTemple. So I'll have to look into going to the most recent version of CentOS, which might break so many different things. Oh well, knowledge is power.

In talking with webchick, it seems that this issue is a edge case. Yet, since mediatemple is a freak'n awesome web hoster it may be a very large edge case (end opinion)

I'll mark it down to major.

CentOS 5.5 still uses the same version of PCRE (6.6-2.el5_1.7), and all the CentOS 5.X versions will in the future use 6.6. CentOS 6 is around the corner (final release Jan 11?), and will have 8.X version of PCRE. We could say that Drupal 6 works best with CentOS 5, and Drupal 7 works best with CentOS 6.

Ubuntu 8.04 has PCRE version 7.4, I believe.

Here's a patch which adds a REQUIREMENT_INFO to the runtime status page if PCRE_VERSION < 7.2.

Since the majority of Drupal functions without issue with an older version of PCRE, I don't think it needs to be a strict requirement.

The exact text and display of the warning should be reviewed by somebody better at writing eloquent error messages.

Guess I'll start reviewing it myself:

+++ modules/system/system.installundefined
@@ -147,6 +147,24 @@ function system_requirements($phase) {
+      $requirements['pcre']['value'] = print_r($pcre_version,1);

This should just be $pcre_version.

If we know the CSS compression causes an error and we can detect the current version of PCRE, maybe we should disable the checkbox on the performance page with a message stating why. Then people wont enable that option and then be left with a website that is un-styled with errors flying around.

From the PHP 5.2.4 release notes:
Key enhancements in PHP 5.2.4 include:
* Upgraded PCRE to version 7.2

So this would mean we need to up the minimum php version to 5.2.4 to do this.

I've been searching for a way to get the PCRE version, but I've been unsuccesful so far: phpversion('pcre') and new ReflectionExtension('pcre')->getVersion() both return FALSE. (the ReflectionExtension class is available since php 5)
There is a constant PCRE_VERSION but ironically, this constant is not available until PHP 5.2.4.

The only way I'm seeing so far is checking the phpinfo() output and grepping through that for the version number.
Can someone who currently has PCRE < 7.2 check that the version number is present in phpinfo(), preferably looking a bit like this (output of new ReflectionExtension('pcre')->info())
PCRE

cross post, but setting to needs work because the constant PCRE_VERSION used in the patch does not exist in PHP < 5.2.4, but Drupal 7 only requires 5.2.0.

I was wrong, currently 5.2.4 is required for drupal 7 (it's confusing, since it has changed back and forth quite a few times).

+++ modules/system/system.install
@@ -147,6 +147,24 @@ function system_requirements($phase) {
+  if ($phase == 'runtime' && extension_loaded('pcre')) {

There is no reason this could not be checked at install time already.

+++ modules/system/system.install
@@ -147,6 +147,24 @@ function system_requirements($phase) {
+    list($pcre_version) = explode(' ', PCRE_VERSION, 2);
+    if (version_compare($pcre_version, $minimum_suggested_pcre_version) < 0) {

version_compare(PCRE_VERSION, $minimum_suggested_pcre_version) < 0 does the trick as well.

+++ modules/system/system.install
@@ -147,6 +147,24 @@ function system_requirements($phase) {
+      $requirements['pcre']['severity'] = REQUIREMENT_INFO;

The advantage of making this a hard requirement is that both core and contrib can assume it's there and works, not having to build workarounds or special casing the version. PCRE 7.2 is included with PHP 5.2.4, so this should be a rare situation.

+++ modules/system/system.install
@@ -147,6 +147,24 @@ function system_requirements($phase) {
+      $requirements['pcre']['value'] = print_r($pcre_version,1);

PCRE_VERSION is also appropriate here.

+++ modules/system/system.install
@@ -147,6 +147,24 @@ function system_requirements($phase) {
+      $requirements['pcre']['description'] = $t('Drupal suggests the <a href="@pcre_extension">PCRE extension</a> in PHP be at least version @minimum_suggested_pcre_version.  Some features may not function properly.  See the <a href="@system_requirements">system requirements page</a> for more information.', array(

This wording is adapted from the PHP version requirement:

Your <a href="@pcre_extension">PCRE extension</a> is too old. Drupal requires at least PCRE @minimum_suggested_pcre_version.

Powered by Dreditor.

version_compare(PCRE_VERSION, $minimum_suggested_pcre_version) < 0 does not do the trick, as for example

version_compare('7.2 19-Jun-07', '7.2') < 0

returns TRUE.

The remainder of your suggestions in #20 should have been addressed.

Setting to needs review.

+++ modules/system/system.install
@@ -147,6 +147,24 @@ function system_requirements($phase) {
+      $requirements['pcre']['description'] = $t('Your <a href="@pcre_extension">PCRE extension</a> is too old. Drupal requires at least PCRE @minimum_pcre_version.  See the <a href="@system_requirements">system requirements page</a> for more information.', array(

There should be only one space separating sentences. Other than that, it looks good to me.

Powered by Dreditor.

Updated patch to fix the "There should be only one space separating sentences." comment in #23.

Just a quick note. My CentOS system which has somehow been upgraded to PHP 5.2 and I forget how (possibly through a srpm recompile from a fedora core system) has PCRE 6.6 -- so this would require me to somehow upgrade that along with PHP.

While the PHP 5.2 argument is pretty solid (sadly) I have trouble believing we NEED PCRE 7.2 for this feature and would plead that we should fix the regex instead.

Sites running CentOS/RHEL 5 can stay on Drupal 6 until they upgrade to CentOS/RHEL 6. RHEL 6 is already out, and CentOS 6 should be out in January.

Oops. Didn't mean to change status.

I'm in agreement with merlinofchaos. RHEL/CentOS 5 has a sizable adoption rate and it could significantly slow down D7 adoption by alienating these users. A large company like MediaTemple can't just flick a switch and upgrade to RHEL 6, such a migration would be measured in months. Fixing the regex should be a high priority (IMHO).

Sites running CentOS/RHEL 5 can stay on Drupal 6 until they upgrade to CentOS/RHEL 6

Words cannot describe how I feel about this statement. Given that I've been contributing to Drupal 7 for quite some time on CentOS 5, I think this is a ridiculous thing to even suggest that suddenly I should be excluded from Drupal 7 contribution over a regular expression.

Well, I see three options to move forward:

1. Strictly require PCRE >= 7.2, and use the patch in #24. This seems to not have much support due to lack of inclusion in CentOS 5.x. It's a bit unfortunate since PCRE 7.2 is, at this point, over three years old and is included with the minimum version of PHP we are requiring. (Although, you can compile PHP with older external versions of PCRE libraries as CentOS must be doing).
2. Recommend PCRE >= 7.2. Either put a note on the requirements page like we do for the upload progress functionality or disable the offending options in the performance tab relating to CSS optimization. The patch in #14 would be a starting point, but would need some work depending on the direction we'd like to head.
3. Rewrite the regex. Reopen #460448: Some CSS rules are broken once CSS aggregation is enabled and rewrite the regex to not use the (?| construction. I'm no regex guru so it's not something I can help with. This option is favored by @merlinofchaos, @justinrandell, @ngmaloney, etc.

I guess I think the last option is the "best," but if the regex isn't rewritten we should go with #2.

#460448: Some CSS rules are broken once CSS aggregation is enabled is where the action is.