Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

fixed pcre named capture bug from ticket #1763 #681

Closed
wants to merge 1 commit into from

7 participants

@b7kich

Use backwards compatible named capture, i.e. "(?P...)" instead of "(?...)"

Fixes
http://cakephp.lighthouseapp.com/projects/42648/tickets/1763-php-warning-in-libcakeconsoleconsoleoutputphp-line-186

See changelog in http://de2.php.net/manual/en/function.preg-match.php

I was still having this issue with the latest standard CentOs packages php53-5.3.3-7.el5_8 and pcre-6.6-6.el5_6.1 despite the claim that named capture options were fixed in PHP 5.2.2

@AD7six
Collaborator

CakePHP doesn't support versions < 5.2.8, but this problem is caused by having broken pcre libraries - which you have.

As mentioned by ADmad in the (closed) ticket you've referenced, there's more detail about how to fix broken pcre libs in the comments here.

I don't think it's the responsibility of CakePHP to work around Centos' borked/outdated pcre package - there will be other problems in the core caused by that too, of most relevance and significance: cache-confusion with file based caching. PCRE 6.6 is nearly 7 years old and nowhere near a recent release.

For future reference please submit pull requests against the active development branch (2.2 at the time of writing).

@AD7six AD7six closed this
@b7kich

I am using the current CentOS PHP 5.3 package (php53-5.3.3-7.el5_8 from Mon 07 May 2012).

This is the only place where named capture is used in the whole codebase:
[b7kich@centos cakephp]# grep -r '[(][?][<][^!=]' .
./lib/Cake/Console/ConsoleOutput.php: '/<(?[a-z0-9-_]+)>(?.*?)<\/(\1)>/ims', array($this, '_replaceTags'), $text
Binary file ./.git/objects/pack/pack-cf421ac2eeba1dd67921d13dbf9c923a3caf9a80.pack matches

I know that PCRE is outdated, but this simple change fixes a bug that exists on all CentOS 5 and RHEL5 boxes.

@b7kich

I saw the #582 - it's very old and related to cakephp 1.3.4.I'm not saying cakephp is responsible for any outdated borked configuration out there.

I've just submitted a simple fix for a common issue.

@AD7six
Collaborator

That's like giving a car with 3 wheels a tuneup.

@b7kich

You're right. Stupid of me to send a bugfix for cakephp.

@AD7six
Collaborator

It's not a mistake to send bugfixes to CakePHP

But there is no supported install where this change fixes something (therefore it's a change with no benefits), and to be affected by what you report there are much bigger problems with your cake install. Trivial though the change may be you're just delaying fixing your pcre - the correct solution as indicated in both tickets referenced above.

@b7kich

That's just stupid FUD. Stop suggesting I am running an outdated OS or unsupported PHP version.

Unlike you I know exactly what I'm doing AND I read before I post.

@suzuki

I like this patch.
I used same patch in my CentOS 5.8 and php53-5.3.3-7.el5_8.

In Japan, the CentOS5 is running on many servers, yet.

@markstory
Owner

Instead of using named capture groups, why not send a patch to remove the named capture groups? Its not like they are a critical component to the feature/behaviour.

@AD7six
Collaborator

That's just stupid FUD. Stop suggesting I am running an outdated OS or unsupported PHP version.

If that's what you're reading you're entirely missing the point. This is the only thing of relevance I see in you ticket description:

pcre-6.6-6.el5_6.1

You're using an old and outdated version of pcre. That's the only problem.

Look at the version table for pcre http://www.php.net/manual/en/pcre.installation.php You have php 5.3 and the version of pcre that came with php 5.1.x - that's why you see problems which were addressed in 5.2, because the version of the relevant installed component you have is still the one that centos installed with php 5.1.x.

Admittedly having double checked with pcre 6.6 the much-more-significant problem I mentioned above (borked multibyte regex support) won't affect you, it was from an earlier pcre 6.x release. But that's still an eon-old version of pcre and avoiding pcre problems by, if you know exactly what you're doing, upgrading pcre to something from this decade should be a cinch.

@b7kich

Thanks suzuki! After I came up with this fix I found that some Japanese people have done it long before I did. Credit goes to:

http://d.hatena.ne.jp/kou_i/20111120/1321778316
http://kurukuru-labo.com/tech/tag/cakephp/

@b7kich

@markstory - the named capture in the backwards compatible version will work for all supported PHP versions. No point in writing more code - which AD7six claims is the wrong way to fix the issue in the first place.

@b7kich

I don't have a problem Andy, I have already forked and patched the cakephp sources for my purposes. I'm just offering this to help other people.

Either you don't understand or are uncertain what the submitted change does (and it's fairly simple, being two characters). In this case you should have passed this pull request on to someone who does.

Otherwise you'd actually know there is no cost associated in reformulating the named capture this form.

Still you advocate that the cost of upgrading thousands of CentOS5 boxes to a new pcre version is better. Not only do we not have access to all these boxes, there is also the risk of an updated pcre breaking some other package. Now before we're getting back to you saying that's none of cakephp's business, ask yourself to what greater good you're opposing this change.

@markstory
Owner

I think arguing over a few P is a bit silly personally, and think we can all come up with better things to argue about :). I don't really see a real reason to not do this change.

@markstory markstory referenced this pull request from a commit
@markstory markstory Change named capture group syntax.
This increases compatibility with really old version of
PCRE used on CentOS.

Refs #GH-681
157e243
@markstory
Owner

Fixed in [157e243]

@b7kich

Thanks! :)

@michaelhagedon

Thanks Christian and Mark for fixing this. I'm distributing a new open-source Cake application soon and I just ran into this error. Looking forward to the next release!

@nojimage nojimage referenced this pull request from a commit in nojimage/cakephp
@nojimage nojimage fixed pcre named capture in Hash class
refs #GH-681
32c01fb
@ghost Unknown referenced this pull request in rwetzlmayr/textpattern
Closed

PCRE named groups incompatibility breaks parseURI() #264

@ghost Unknown referenced this pull request in textpattern/textpattern
Closed

PCRE named groups incompatibility breaks parseURI() #268

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jun 1, 2012
  1. @b7kich
This page is out of date. Refresh to see the latest.
Showing with 1 addition and 1 deletion.
  1. +1 −1  lib/Cake/Console/ConsoleOutput.php
View
2  lib/Cake/Console/ConsoleOutput.php
@@ -182,7 +182,7 @@ public function styleText($text) {
return preg_replace('#</?(?:' . $tags . ')>#', '', $text);
}
return preg_replace_callback(
- '/<(?<tag>[a-z0-9-_]+)>(?<text>.*?)<\/(\1)>/ims', array($this, '_replaceTags'), $text
+ '/<(?P<tag>[a-z0-9-_]+)>(?P<text>.*?)<\/(\1)>/ims', array($this, '_replaceTags'), $text
);
}
Something went wrong with that request. Please try again.