Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove PCRE2_EXTRA_ALLOW_LOOKAROUND_BSK from pcre compile options #18150

Closed

Conversation

mvorisek
Copy link
Contributor

@mvorisek mvorisek commented Mar 26, 2025

As recommended [1] by main pcre2 maintainer.

The PCRE2_EXTRA_ALLOW_LOOKAROUND_BSK compile option was introduced in dd61002. I belive it was done mainly to fix 1 failing test, not to support some strong usecase. \K in lookaround is not much useful [2]. We should compile pcre2 with default options and remove this semi-deprecated option [3].

[1] PCRE2Project/pcre2#736 (comment)
[2] PCRE2Project/pcre2#736 (comment)
[3] PCRE2Project/pcre2#736 (comment)

@nielsdos
Copy link
Member

I'll need to spend some time this weekend reading the docs and understanding the implications (even if there is little BC concern).
One thing that may be necessary is to guard the flag with a macro-based version check. That's because it's still possible to use an older system libpcre2 version with php master.

@mvorisek
Copy link
Contributor Author

One thing that may be necessary is to guard the flag with a macro-based version check. That's because it's still possible to use an older system libpcre2 version with php master.

I do not think so. The tests are adjusted and since this PR we do not simply pass the semi-deprecated flag.

Copy link
Member

@nielsdos nielsdos left a comment

Choose a reason for hiding this comment

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

Okay. I am fine with this, but please make the following changes to your PR:

  • Add a note in the "backwards incompatibility" section in the UPGRADING file, explaining the change and also linking to the discussion.
  • Squash all commits. Include in your commit description that you're dropping the semi-deprecated flag.

@mvorisek
Copy link
Contributor Author

* Add a note in the "backwards incompatibility" section in the UPGRADING file, explaining the change and also linking to the discussion.

Done, feel free to reword it.

* Squash all commits. Include in your commit description that you're dropping the semi-deprecated flag.

Do you know you can squash merge PR using web UI?

@nielsdos
Copy link
Member

Do you know you can squash merge PR using web UI?

Yes, but since I plan to manually merge it to have a single commit with the news as well, it's more convenient if there already is a single commit with a description.

@nielsdos nielsdos closed this in 355700c Mar 31, 2025
@nielsdos
Copy link
Member

Merged, thanks

@mvorisek mvorisek deleted the pcre_no_PCRE2_EXTRA_ALLOW_LOOKAROUND_BSK branch March 31, 2025 22:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants