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

Add CRLF test case for EregToPregMatchRector #6245

Closed

Conversation

mcenirm
Copy link
Contributor

@mcenirm mcenirm commented Aug 20, 2024

EregToPregMatchRector (and EregToPcreTransformer) convert ereg regular expression strings that contain CR or LF characters (i.e., double-quoted strings with \r or \n) to preg regular expression strings that are single-quoted but contain the unescaped character (i.e., multi-line string literal with embedded CR or LF characters).

Example:

    $splitLF   = split("\n",   $s);
    $splitCRLF = split("\r\n", $s);

... becomes ...

    $splitLF   = preg_split('#
#m',   $s);
    $splitCRLF = preg_split('#
#m', $s);

(The CR is invisible in the second statement in the output, so it looks like a duplicate of the first statement.)

This transformation complies with PHP syntax, and the preg functions treat "#\r#" as equivalent to '#\r#'. The problem occurs when git and its autocrlf behavior removes CR characters without regard to them being embedded in multi-line strings. This results in a platform-dependent regular expression, when the original source code avoided that behavior.

Even though this is clearly a caveat developer situation, it would be nice if the EregToPregMatchRector rule could try to preserve the original representation. Converting the double-quoted string to a suitable single-quoted PCRE regular expression would be ideal, but probably too error prone.

samsonasik added a commit that referenced this pull request Aug 21, 2024
samsonasik added a commit that referenced this pull request Aug 21, 2024
@samsonasik
Copy link
Member

I cherry-picked your commit at PR :

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants