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

fix unescaped slash error in preg match #3513

Conversation

SaidkhojaIftikhor
Copy link

@SaidkhojaIftikhor SaidkhojaIftikhor commented Apr 8, 2023

This is:

  • a bugfix
  • a new feature
  • refactoring
  • additional unit tests

Checklist:

  • Changes are covered by unit tests
    • Changes are covered by existing unit tests
    • New unit tests have been added
  • Code style is respected
  • Commit message explains why the change is made (see https://github.com/erlang/otp/wiki/Writing-good-commit-messages)
  • CHANGELOG.md contains a short summary of the change and a link to the pull request if applicable
  • Documentation is updated as necessary

Why this change is needed?

I faced with an error WARNING preg_match(): Unknown modifier '�'.
After some debuging I found out that preg_quote doesn't escape slash character. I added slash character as a second parameter of preg_quote and now it works :)

image

preg_quote

example code: This code will throw an error because $columnName has a slash in it and preg_quote will not escape it

$columnName = 'Buy/Sell';
$columnId = 'C';
$reference = '[B5]';

$cellReference = $columnId . 5;
$pattern1 = '/\[' . preg_quote($columnName) . '\]/miu';
$pattern2 = '/@' . preg_quote($columnName) . '/miu';

if (preg_match($pattern1, $reference) === 1) {
    $reference = preg_replace($pattern1, $cellReference, $reference);
} elseif (preg_match($pattern2, $reference) === 1) {
    $reference = preg_replace($pattern2, $cellReference, $reference);
}

@SaidkhojaIftikhor SaidkhojaIftikhor changed the title Iftikhor/fix preg match error fix unescaped slash error in preg match Apr 8, 2023
Copy link
Member

Choose a reason for hiding this comment

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

The crux of the PR; and I've verified that the change is necessary her.
Can you provide a unit test that will demonstrate the issue, and prove that this fix works?

Copy link
Author

@SaidkhojaIftikhor SaidkhojaIftikhor Apr 10, 2023

Choose a reason for hiding this comment

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

Ok, I will try. I am new to this repository and I don't understand a lot of things, that's why I added delimiter everywhere in code.

How can I learn the codebase ? I tried to read the code but it's kinda hard

Copy link
Member

Choose a reason for hiding this comment

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

As with the Structured Reference changes, can you provide unit tests to verify that this is a problem, and that the solution does resolve that problem for all these instances where you've made the change?

@SaidkhojaIftikhor SaidkhojaIftikhor marked this pull request as draft May 3, 2023 10:51
oleibman added a commit to oleibman/PhpSpreadsheet that referenced this pull request May 24, 2023
PR PHPOffice#3513, developed by @SaidkhojaIftikhor, has been stuck for some time awaiting tests. This is the first of three PRs to replace that one. This accomodates the use of slash as a delimiter in functions TEXTAFTER, TEXTBEFORE, TEXTSPLIT, and NUMBERVALUE. The source changes are very simple. Additional tests exercise all the source changes.
oleibman added a commit to oleibman/PhpSpreadsheet that referenced this pull request May 24, 2023
PR PHPOffice#3513, developed by @SaidkhojaIftikhor, has been stuck for some time awaiting tests. This is the second of three PRs to replace that one. This accomodates the use of slash as a delimiter in column names in Tables and Structured References. The source changes are very simple. Additional tests exercise all the source changes.

There is also a preg_quote call when a table is renamed. I have also changed it to accomodate slash, because it's the right thing to do. But ... I can't think how to test it. PhpSpreadsheet will not allow you to set a table name to a string containing a slash (a test is added to confirm), and, if I manually update the Xml in an Xlsx spreadsheet so that the name does contain a slash, Excel will, understandably, complain that the file is corrupt.
oleibman added a commit to oleibman/PhpSpreadsheet that referenced this pull request May 24, 2023
PR PHPOffice#3513, developed by @SaidkhojaIftikhor, has been stuck for some time awaiting tests. This is the first of three PRs to replace that one. This PR also allows the use of slash as a thousands separator or decimal separator or currency symbol. These are, of course, very unusual situations; the main reason to support them is so that PhpSpreadsheet code will not crash when users set those options. New Calculation/Engine and AdvancedValueBinder tests confirm these. While making these changes, a few errors were found in AdvancedValueBinder and Calculation/Engine/FormattedNumber, e.g. currencies weren't parsed correctly when period was used as the group separator and comma as the decimal separator. Tests have been added for those situations.
@oleibman
Copy link
Collaborator

I looked into the testing that would be needed, and it seems pretty complicated, so complicated that I have just submitted 3 different PRs to handle it. If those PRs are merged, I will close this one.

oleibman added a commit that referenced this pull request May 27, 2023
PR #3513, developed by @SaidkhojaIftikhor, has been stuck for some time awaiting tests. This is the first of three PRs to replace that one. This accomodates the use of slash as a delimiter in functions TEXTAFTER, TEXTBEFORE, TEXTSPLIT, and NUMBERVALUE. The source changes are very simple. Additional tests exercise all the source changes.
oleibman added a commit that referenced this pull request May 27, 2023
…es (#3583)

PR #3513, developed by @SaidkhojaIftikhor, has been stuck for some time awaiting tests. This is the second of three PRs to replace that one. This accomodates the use of slash as a delimiter in column names in Tables and Structured References. The source changes are very simple. Additional tests exercise all the source changes.

There is also a preg_quote call when a table is renamed. I have also changed it to accomodate slash, because it's the right thing to do. But ... I can't think how to test it. PhpSpreadsheet will not allow you to set a table name to a string containing a slash (a test is added to confirm), and, if I manually update the Xml in an Xlsx spreadsheet so that the name does contain a slash, Excel will, understandably, complain that the file is corrupt.
oleibman added a commit that referenced this pull request May 27, 2023
PR #3513, developed by @SaidkhojaIftikhor, has been stuck for some time awaiting tests. This is the first of three PRs to replace that one. This PR also allows the use of slash as a thousands separator or decimal separator or currency symbol. These are, of course, very unusual situations; the main reason to support them is so that PhpSpreadsheet code will not crash when users set those options. New Calculation/Engine and AdvancedValueBinder tests confirm these. While making these changes, a few errors were found in AdvancedValueBinder and Calculation/Engine/FormattedNumber, e.g. currencies weren't parsed correctly when period was used as the group separator and comma as the decimal separator. Tests have been added for those situations.
@oleibman
Copy link
Collaborator

The replacement PRs are merged. Closing this one.

@oleibman oleibman closed this May 27, 2023
@SaidkhojaIftikhor SaidkhojaIftikhor deleted the iftikhor/fix-preg-match-error branch June 10, 2024 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants