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 issue where running FileLocator::getClassname() on a directory would cause a PHP error #8216

Merged
merged 2 commits into from
Nov 19, 2023

Conversation

colethorsen
Copy link
Contributor

@colethorsen colethorsen commented Nov 14, 2023

fix issue where running getClassName on a directory would cause a PHP

Description

This issue specifically comes up when you put commands in folders for better organization, the previous setup used findQualifiedNameFromPath instead of getClassname, getClassname has no checking to see if what is passed in is a directory, which could easily happen as listFiles can return directories as well as files.

An uncaught Exception was encountered

Type:        ErrorException
Message:     file_get_contents(): read of 8192 bytes failed with errno=21 Is a directory
Filename:    vendor/codeigniter4/framework/system/Autoloader/FileLocator.php
Line Number: 122

Backtrace:
-122 - vendor/codeigniter4/framework/system/Autoloader/FileLocator.php::file_get_contents
-104 - vendor/codeigniter4/framework/system/CLI/Commands.php::getClassname
-46 - vendor/codeigniter4/framework/system/CLI/Commands.php::discoverCommands
-174 - vendor/codeigniter4/framework/system/Config/Services.php::__construct
-258 - vendor/codeigniter4/framework/system/Config/BaseService.php::commands
-199 - vendor/codeigniter4/framework/system/Config/BaseService.php::__callStatic
-171 - vendor/codeigniter4/framework/system/Config/Services.php::getSharedInstance
-258 - vendor/codeigniter4/framework/system/Config/BaseService.php::commands
-41 - vendor/codeigniter4/framework/system/CLI/Console.php::__callStatic
-103 - spark::run

Potential other solutions to this would be to prevent listFiles from returning directories, or filtering the list of files prior to running getClassname in discoverCommands

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@kenjis kenjis added bug Verified issues on the current code behavior or pull requests that will fix them and removed bug Verified issues on the current code behavior or pull requests that will fix them labels Nov 14, 2023
@kenjis
Copy link
Member

kenjis commented Nov 14, 2023

How can we reproduce the error?

It seems listFiles() does not return directories. See the last param false:

$tempFiles = get_filenames($fullPath, true, false, false);

@@ -119,6 +119,10 @@ public function locateFile(string $file, ?string $folder = null, string $ext = '
*/
public function getClassname(string $file): string
{
if(is_dir($file)) {
return '';
}
Copy link
Member

Choose a reason for hiding this comment

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

I think this code is okay (if the coding style is fixed), because a directory is never a class.

@colethorsen
Copy link
Contributor Author

@kenjis you can reproduce the error by following the exception that is posted with a directory in your Commands folder.

a dump of the array $files returned by: $files = $locator->listFiles('Commands/'); in Commands.php on line 92

Produces:

0 => "/app/Commands/testfolder"                           //directory
1 => "/app/Commands/testfolder/testing.php"               //file
2 => "/app/Commands/testing.php"                          //file
3 => "/base/Commands/AccountAddresses"                    //directory
4 => "/base/Commands/AccountAddresses/ErrorCleanup.php"   //file
5 => "/base/Commands/Accounts"                            //symlink
6 => "/base/Commands/Accounts/TokenCleanup.php"           //file
7 => "/base/Commands/ActivityLogMaintain.php"             //file
8 => "/base/Commands/Assets"                              //symlink
9 => "/base/Commands/Assets/UnusedCheck.php"              //file

So there could be a bug with the get_filenames function you've referenced.

This is running on PHP 7.4.21

@colethorsen
Copy link
Contributor Author

Sorry, I had a modification made to the get_filenames so that it follows symlinks as well which wasn't updated with the additional include directories parameter. Its working now, still probably a worthwhile check to be making though.

As a sidenote, should get_filenames just follow symlinks by default or as a global config option?

@kenjis
Copy link
Member

kenjis commented Nov 16, 2023

get_filenames() follows symlinks by default.

$ ls -l
total 16
-rw-r--r--  1 kenji  staff  1579 10 27 16:48 BaseController.php
-rw-r--r--  1 kenji  staff   204 11 16 08:59 Home.php
lrwxr-xr-x  1 kenji  staff     8 11 16 09:00 Symlink.php -> Home.php
<?php

namespace App\Controllers;

class Home extends BaseController
{
    public function index(): string
    {
        helper('filesystem');
        dd(get_filenames(APPPATH . 'Controllers/'));
    }
}

Screenshot 2023-11-16 9 02 11

@kenjis kenjis added the bug Verified issues on the current code behavior or pull requests that will fix them label Nov 16, 2023
@colethorsen
Copy link
Contributor Author

get_filenames() follows symlinks by default.

It finds symlinks to files that are directly within the structure, but if a folder is symlinked it won't go into the folder due to the configuration of the RecursiveDirectoryIterator it would need to change from:

new RecursiveDirectoryIterator($sourceDir, RecursiveDirectoryIterator::SKIP_DOTS),

to

new RecursiveDirectoryIterator($sourceDir, RecursiveDirectoryIterator::SKIP_DOTS | FilesystemIterator::FOLLOW_SYMLINKS),

@kenjis kenjis changed the title fix issue where running getClassName on a directory would cause a PHP error fix issue where running FileLocator::getClassName() on a directory would cause a PHP error Nov 16, 2023
@kenjis kenjis changed the title fix issue where running FileLocator::getClassName() on a directory would cause a PHP error fix issue where running FileLocator::getClassname() on a directory would cause a PHP error Nov 16, 2023
@kenjis kenjis merged commit 0344aa6 into codeigniter4:develop Nov 19, 2023
61 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Verified issues on the current code behavior or pull requests that will fix them
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants