The function fnmatch is not available in every PHP build. Related core issue: #2620576: fnmatch() is not available on all environments (i.e QNAP QTS).
Running Upgrade Status on Configuration Distribution produces the following:
| File name | Line | Error |
|---|---|---|
| web/modules/contrib/config_distro/modules/config_distro_ignore/src/Plugin/ConfigFilter/DistroIgnoreFilter.php | 138 | Calls to function fnmatch should not exist. |
| web/modules/contrib/config_distro/modules/config_distro_ignore/src/Plugin/ConfigFilter/DistroIgnoreFilter.php | 158 | Calls to function fnmatch should not exist. |
Replace fnmatch() with preg_match().
| Comment | File | Size | Author |
|---|---|---|---|
| #9 | 3144145-replace-fnmatch-with-preg-match-9.patch | 2.11 KB | tadean |
| #6 | 3144145-replace-fnmatch-with-preg-match-6.patch | 2.38 KB | a.dmitriiev |
| #2 | 3144145-replace-fnmatch-with-preg-match-2.patch | 2.42 KB | a.dmitriiev |
Comments
Comment #2
a.dmitriiev commentedPlease review attached patch.
Comment #3
nedjo@a.dmitriiev
Thanks for the patch!
I don't follow it well enough to understand what it's doing or why. The core commit for #2620576: fnmatch() is not available on all environments (i.e QNAP QTS) was mostly single-line substitutions. Can you explain a bit why we need to do more here?
Comment #4
a.dmitriiev commentedI think my change covers all the possible fnmatch cases, maybe it is overdoing ofcourse. Let's stick to the core approach. I will prepare a new patch.
Comment #5
a.dmitriiev commentedComment #6
a.dmitriiev commentedHere is the new patch attached, following core approach.
Comment #7
joegraduateComment #8
joegraduateComment #9
tadeanThank you for these great patches @a.dmitriiev !
I tested both of these patches out with the
config_distro_ignorefunctionality. In my testing it appears that the patch in #3144145-6: Consider replacing fnmatch usage has an issue where any ignore rule will match any config name (e.g.foowill matchnode.type.my_content_type). The original patch in #3144145-2: Consider replacing fnmatch usage seemed to work in almost all cases but has a minor issue where a partial match now matches a complete config name (e.g.contentmatchesnode.type.my_content_typebecause the generated regex pattern was missing the^$I have updated the patch in #3144145-2: Consider replacing fnmatch usage to fix the substring issue, use
preg_quote, and for coding standards. I have removed the case insensitivity flag becausefnmatch()wasn't originally being used withFNM_CASEFOLD. I think in some cases (windows platforms, maybe?) the original implementation usingfnmatch()might have been case insensitive in some places and not others.Most of the
fnmatch()replacements in Drupal have not been a complete reimplementation of the shell-style patterns as regex, but this patch retains the wildcard functionality on * and ?, which I think is useful for theconfig_distro_ignoreuse case.Comment #10
joegraduatePatch in #9 looks good to me. Thanks @tadean!
Discussed this offline with @tadean and we are in agreement that it would be good to merge this ahead of an alpha release with D10 compatibility and create a follow-up issue that adds automated test coverage for this functionality before creating a beta release.
Comment #11
trackleft2Tested with many patterns using the test module after changing config within the
core.entity_form_display.node.sample_content_type.default.ymlfile including:Comment #13
joegraduateCommitted to 2.0.x. Thanks all!