fnmatch() is not available on all environments (i.e QNAP QTS)

fnmatch() is a built in PHP function: http://php.net/manual/en/function.fnmatch.php - though there is a note "Warning: For now, this function is not available on non-POSIX compliant systems except Windows."

What server/hosting platform and version of PHP are you using?

@chapf that's probably because the other CMS doesn't use fnmatch()

We can add a warning to the installer if the function isn't available, and add a note to http://drupal.org/requirements that it's required.

The PHP docs comments have suggested polyfills for when it's not available, could potentially look at that as well.

A requirements check patch.

#8 may not work if the installer can't make it there. This check may have to go in install.php the way it was done in #2421451-14: Drupal needs comments in opcache.

@cilefen I think it might be possible to not use this function and use preg_match() or something similar.

We could then open a followup to add this to the list of functions that the phpcs rules provided by Coder say we shouldn't use.

This is one of six usages.

This is still wrong and the config import test will fail.

Created #2630270: fnmatch is not available on non-posix php builds (apart Windows) - let's add it to the list of not allowed methods

This patch does not work for D8.0.3 because fnmatch() is now also called at
FileStorage.php line 211.

Perhaps it's time to fix this so those of us who cannot use fnmatch() don't have to patch every new version.

@rsbecker I am a little busy today but would you like to create the patch so this can be fixed?

+++ b/core/lib/Drupal/Core/Config/FileStorage.php
@@ -207,7 +207,8 @@ public function listAll($prefix = '') {
-      if ($file[0] !== '.' && fnmatch($prefix . '*' . $extension, $file)) {
+      $pattern = '/^' . strtr(preg_quote($prefix . '*' . $extension, '/'), array('\*' => '.*')) . '$/i';
+      if ($file[0] !== '.' && preg_match($pattern, $file)) {

@rsbecker Not sure what you mean. The patch in #17 contains the fix to FileStorage. The issue needs a review to get done.

Sorry. I made a mistake. There is now a third place.

In the FileStorage.php file that shipped with 8.0.3, the changes are at 211, 222 and 314.

That is not what I see.

drupal8x ((8.0.3) %)$ ack fnmatch core
core/lib/Drupal/Core/Config/FileStorage.php
211:      if ($file[0] !== '.' && fnmatch($prefix . '*' . $extension, $file)) {
312:          if ($file[0] !== '.' && fnmatch('*.' . $this->getFileExtension(), $file)) {

# ... more files

With the patch in #17 applied I can't find any usages in core.

This looks great. Let's get this done.

+++ b/core/lib/Drupal/Core/Config/FileStorage.php
@@ -207,7 +207,8 @@ public function listAll($prefix = '') {
+      $pattern = '/^' . strtr(preg_quote($prefix . '*' . $extension, '/'), array('\*' => '.*')) . '$/i';

@@ -308,7 +309,8 @@ protected function getAllCollectionNamesHelper($directory) {
+          $pattern = '/^' . strtr(preg_quote('*.' . $this->getFileExtension(), '/'), array('\*' => '.*')) . '$/i';

+++ b/core/lib/Drupal/Core/Config/InstallStorage.php
@@ -203,7 +203,8 @@ public function getComponentNames(array $list) {
+          $pattern = '/^' . strtr(preg_quote('*' . $extension, '/'), array('\*' => '.*')) . '$/i';

@@ -230,7 +231,8 @@ public function getCoreNames() {
+        $pattern = '/^' . strtr(preg_quote('*' . $extension, '/'), array('\*' => '.*')) . '$/i';

I think the usage of strtr() makes this a bit complex to read...

Further to #27:

• Also realised that we don't have to create the patterns inside a loop - saving CPU and making it more obvious what is going on here.
• And, for example, /^.*\.yml$/ since what we want is just to check that the filename ends in .yml. The pattern for that is /\.yml$/

So what actually provides this functionality. Is this some form of compiler flag?

Warning
For now, this function is not available on non-POSIX compliant systems except Windows.

It seems to be that QNAP for example strips down their PHP binary as much as possible I would guess there are more problems beside this function,
but let's see.

+++ b/core/lib/Drupal/Core/Config/FileStorage.php
@@ -290,6 +291,7 @@ public function getAllCollectionNames() {
+    $pattern = '/\.' . preg_quote($this->getFileExtension(), '/') . '$/i';
     foreach (new \DirectoryIterator($directory) as $fileinfo) {
       if ($fileinfo->isDir() && !$fileinfo->isDot()) {
         $collection = $fileinfo->getFilename();
@@ -309,7 +311,7 @@ protected function getAllCollectionNamesHelper($directory) {

@@ -309,7 +311,7 @@ protected function getAllCollectionNamesHelper($directory) {
         // collection.
         // @see \Drupal\Core\Config\FileStorage::listAll()
         foreach (scandir($directory . '/' . $collection) as $file) {
-          if ($file[0] !== '.' && fnmatch('*.' . $this->getFileExtension(), $file)) {
+          if ($file[0] !== '.' && preg_match($pattern, $file)) {

Nice simplification of the regex

Thanks @cilefen, @dawehner, and @alexpott for working on this!

+++ b/core/lib/Drupal/Core/Config/FileStorage.php
@@ -207,8 +207,9 @@ public function listAll($prefix = '') {
     $names = array();
+    $pattern = '/^' . preg_quote($prefix, '/') . '.*' . preg_quote($extension, '/') . '$/i';

@@ -290,6 +291,7 @@ public function getAllCollectionNames() {
+    $pattern = '/\.' . preg_quote($this->getFileExtension(), '/') . '$/i';

+++ b/core/lib/Drupal/Core/Config/InstallStorage.php
@@ -190,6 +190,7 @@ protected function getAllFolders() {
+    $pattern = '/' . preg_quote($extension, '/') . '$/i';

@@ -220,6 +221,7 @@ public function getComponentNames(array $list) {
+    $pattern = '/' . preg_quote($extension, '/') . '$/i';

So I looked at this and noticed we are making it case-insensitive (the /i). Since case sensitivity definitely differs from environment to environment and so could have different results, I asked @alexpott about this choice. We looked at fnmatch() and it does have a flag for case insensitivity, but it is not the default, so this is changing the behavior. Marking NW for that.

This also makes me think we should have test coverage for what we expect the results of these filename matches to be -- i.e., factor them out into a method, then have some data providers with unit tests for some different patterns. It doesn't necessarily need to block this issue, but it's worth discussing whether it should.

@alexpott also suggested it could be three separate issues for three separate tests: one for the config storages, one for Migrate, and one for CKeditor.

Wow, impressive detective work @xjm!

I'm generally not a huge fan of the so-called "Drupalisms", but does it make sense to add a utility helper function for this á la \Drupal\Component\Utility\Compatibility::fnMatch() (namespace/function name TBD, of course) ? Not sure myself, as this seems less of a common use-case than e.g. \Drupal\Component\Utility\Unicode::strlen() and friends, but OTOH it's one-less thing contrib authors would have to copy-paste. Thoughts?

So yep #30 is correct the i is wrong. I think given that we're not changing behaviour in this issue we don't need to add extensive additional tests here BUT a followup issue sounds a good idea. Given the importance of the config FileStorage I've improved FileStorageTest to cover the untested assumptions in HEAD and this patch. HOWEVER, i suspect that config's database storage is case insensitive - which can be confirmed by doing \Drupal::service('config.storage')->listAll('System'); and yep with a mysql or even postgres backend it is (because we're using a like query). So I think there should be a followup to make config storages behave the same way. Nice catch @xjm.

Just to confirm fnmatch() is case sensitive I did...

>>> fnmatch('*.jpg', 'alex.JPG');
=> false

>>> fnmatch('*.JPG', 'alex.JPG');
=> true

@tstoeckler yeah I considered that too... but still a contrib author would have to learn the fnmatch needs to be Comptability::fnmatch() and we already added a warning to coder... #2630270: fnmatch is not available on non-posix php builds (apart Windows) - let's add it to the list of not allowed methods

Added #2666954: StorageInterface::listAll() implementations are different as followup based on #33.

+++ b/core/modules/config/src/Tests/Storage/FileStorageTest.php
@@ -70,6 +70,9 @@ public function testlistAll() {
+    $this->assertIdentical(['system.performance'], $this->storage->listAll('system'), 'The FileStorage::listAll() with prefix works.');
+    $this->assertIdentical([], $this->storage->listAll('System'), 'The FileStorage::listAll() is case sensitive.');

Should we put an @todo for #2666954: StorageInterface::listAll() implementations are different here?

@xjm I don't think so - depending on what we choose the tests will either break or continue to be affective.

@alexpott I'd agree with you if the title of this issue were something like "Config storage is case-sensitive". However, it is not directly in scope here. For that reason, to me, one shouldn't be expected to git blame to know why the test is there. So I'd prefer a comment explaining that it's a known issue that the case sensitivity does not extend to certain databases. :)

Edit: or in other words, there is a difference between why the test is there and why the test was added. VCS should be used for the latter. The former however needs code documentation, and we all know about @todos without issues. ;)

Not a big deal either way I guess, but meh. At least having the test coverage is a big step. What it doesn't cover though is the database-specific case insensitivity part targeted for the followup (and not this issue, therefore not in the commit history).

Anyway, I agree that retaining the case-sensitive behavior seems like the correct course for now.

Adding an @todo

The regular expressions all look okay to me and I think this covers @xjm's concern from #37 now.

#2671708: Add a RegexDirectoryIterator is related

Studied the regexes again and this looks good to me. Since this is fixing a major bug, let's get it into both 8.0.x and 8.1.x now. Then we can follow up with #2671708: Add a RegexDirectoryIterator and using that in 8.1.x.

Thanks @chapf for reporting this issue! Committed to 8.1.x and cherry-picked to 8.0.x