In #3516706-83: Disallow dangerous filenames e.g. command injection characters @ghost of Drupal past pointed out that \Drupal\Component\Utility\Unicode::PREG_CLASS_WORD_BOUNDARY maintained a discrete list of characters rather than using the \p character class patterns.
This was because of a #604002-54: Poor search support of some Unicode scripts bug found with \p 17 years ago.
That bug may no longer be present
Adapt SearchTextProcessorTest from search module into a unit test for Unicode word boundaries
Simplify the pattern on \Drupal\Component\Utility\Unicode::PREG_CLASS_WORD_BOUNDARY and see if tests pass
| Comment | File | Size | Author |
|---|
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #3
larowlanThe UnicodeTest still passes with this MR - let's see what else breaks
Comment #4
ghost of drupal pastThe problem is not UnicodeTest, the real test ended up in SearchTextProcessorTest which I understand is to be moved into contrib. Since parent got committed, the \p test is now security weight bearing and as such UnicodeTest.txt must be moved to core and the \p test added to UnicodeTest.
Comment #5
larowlanPushed the search test module fixture and matching unit test case.
There are 4 fails:
- U+06DE ARABIC START OF RUB EL HIZB is now Other_Symbol - so that's different to how we were classifying it - should be stripped as its a symbol.
- U+0CF1, U+0CF2 (Kannada signs) - these are now Other_Letter - we were stripping but should be retained now
- U+17B4, U+17B5 (Khmer inherent vowels) - these are now NonSpacing_Mark so should be retained
- U+3248-U+324F (circled numbers on black square) these are now Other_Number so should be retained
Comment #6
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #7
ghost of drupal pastPerhaps the issue title should reflect the fact you are exploring whether \p is bug free this time because security. That we have a test for this already for PREG_CLASS_WORD_BOUNDARY is a useful side effect but not the point.
Comment #8
larowlanSure
So for the four fails I think we need to update the text fixture to match the current classification of those characters
Comment #9
xjmThis is definitely going to need eyes. :)