Unset Composer's autoloader from the autoload stack by paulbalandan · Pull Request #5834 · codeigniter4/CodeIgniter4

paulbalandan · 2022-03-26T17:24:29Z

Since we only need Composer to register its PSR4 namespaces and classmaps into our internal autoloader, it should not be in the spl_autoload stack after as this messes with class_exists checks. When our autoloader fails to find the file, PHP passes the search to the next autoloader, which is Composer. Our internal autoloader uses include_once while Composer uses include causing the redeclaration error.

Cannot write a unit test for this. To check manually using this repo:

Add "App\\": "app" in the autoload.psr4 section in the root composer.json
Run composer dump -o to regenerate the class map.
Add 'MyNameSpace' => APPPATH . 'ThirdParty/MyFolder' in Config\Autoload::$psr4 array.
Run php spark make:command TeseCommand --namespace MyNameSpace
Run php spark. This will now error.
Apply this fix ef7dec6
Run php spark. No errors should appear.

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 · 2022-03-27T03:23:08Z

        $paths    = $composer->getPrefixesPsr4();
        $classes  = $composer->getClassMap();

+        $composer->unregister();


This is in the discoverComposerNamespaces() method.
So when $modules->discoverInComposer is false, it does not run.
When $modules->discoverInComposer is false, Composer autoloader is still enabled.
Is it intentional?

Composer's autoloader is only enabled when included in $this->discoverComposerNamespaces(). In the code, once we include COMPOSER_PATH, Composer will register its autoloader and prepend it in the autoload stack. If $modules->discoverInComposer is false, then COMPOSER_PATH is not included and won't be registered.

<?php namespace App\Controllers; use Closure; class Home extends BaseController { public function index() { foreach (spl_autoload_functions() as $autoloader) { switch ($autoloader) { case $autoloader instanceof Closure: echo get_class($autoloader) . PHP_EOL; break; case is_string($autoloader[0]): echo $autoloader[0] . '::' . $autoloader[1] . PHP_EOL; break; case is_object($autoloader[0]): echo get_class($autoloader[0]) . '::' . $autoloader[1] . PHP_EOL; break; } } } }

With this PR, there is no Composer autoloader.

$ php public/index.php CodeIgniter\Autoloader\Autoloader::loadClassmap CodeIgniter\Autoloader\Autoloader::loadClass PHPStan\PharAutoloader::loadClass Closure

Next, make $discoverInComposer false.

--- a/app/Config/Modules.php +++ b/app/Config/Modules.php @@ -29,7 +29,7 @@ class Modules extends BaseModules * * @var bool */ - public $discoverInComposer = true; + public $discoverInComposer = false; /** * --------------------------------------------------------------------------

$ php public/index.php Composer\Autoload\ClassLoader::loadClass CodeIgniter\Autoloader\Autoloader::loadClassmap CodeIgniter\Autoloader\Autoloader::loadClass PHPStan\PharAutoloader::loadClass Closure

Composer autoloader is registered.

Okay. So there's a place in the application lifecycle where Composer is included regardless of the $discoverInComposer flag. I'll take a look into it but most probably it shouldn't be included automatically.

Found it!

In system/bootstrap.php and system/Test/bootstrap.php, we require_once COMPOSER_PATH regardless of the $discoverInComposer flag, which shouldn't be the case as it is handled already in the Services::autoloader()->initialize() call.

if (is_file(COMPOSER_PATH)) { require_once COMPOSER_PATH; }

kenjis · 2022-03-27T03:36:51Z

Since we only need Composer to register its PSR4 namespaces and classmaps into our internal autoloader,

When a user does not have Composer classmap (don't use -o) and sets discoverInComposer false,
we need Composer autoloader to find classes in Composer packages.

So this PR is okay unless there is no bugs in the CI4 autoloader of PSR-4 class loading?

kenjis · 2022-03-27T03:47:01Z

@paulbalandan This is not directly related this PR, but I think the following logic is the cause of the error,
and should be changed.

$locator->listFiles() uses psr-4 namespaces to find files, but returns file paths only, no namespaces.
And from the file path list, findQualifiedNameFromPath() guesses full classname. So the namespace of the class might be incorrect.

CodeIgniter4/system/CLI/Commands.php

Lines 90 to 103 in 7107e78

    
           /** @var FileLocator $locator */ 
        
           $locator = service('locator'); 
        
           $files   = $locator->listFiles('Commands/'); 
        
           // If no matching command files were found, bail 
        
           // This should never happen in unit testing. 
        
           if ($files === []) { 
        
               return; // @codeCoverageIgnore 
        
           } 
        
           // Loop over each file checking to see if a command with that 
        
           // alias exists in the class. 
        
           foreach ($files as $file) { 
        
               $className = $locator->findQualifiedNameFromPath($file);

MGatner · 2022-03-27T11:38:03Z

Looks like good work and review underway already, let me know if you need me.

paulbalandan · 2022-03-28T13:41:42Z

$locator->listFiles() uses psr-4 namespaces to find files, but returns file paths only, no namespaces. And from the file path list, findQualifiedNameFromPath() guesses full classname. So the namespace of the class might be incorrect.

findQualifiedNameFromPath() uses class_exists() check on the guessed class name to see if it is a real class.

kenjis · 2022-03-28T22:03:50Z

@paulbalandan If you disable $discoverInComposer, you can't autoload Composer package files.
Is this what you want?

Auto-Discovery is the functionality to find automatically different type files like Command, Routes.php. $discoverInComposer means whether to find them in Composer PSR-4 namespaces.
I don't think it disables Composer autoloader.

--- a/app/Config/Modules.php
+++ b/app/Config/Modules.php
@@ -29,7 +29,7 @@ class Modules extends BaseModules
      *
      * @var bool
      */
-    public $discoverInComposer = true;
+    public $discoverInComposer = false;
 
     /**
      * --------------------------------------------------------------------------

--- a/app/Controllers/Home.php
+++ b/app/Controllers/Home.php
@@ -6,6 +6,6 @@ class Home extends BaseController
 {
     public function index()
     {
-        return view('welcome_message');
+        $faker = \Faker\Factory::create();
     }
 }

$ php public/index.php 


[Error]

Class "Faker\Factory" not found

at APPPATH/Controllers/Home.php:9

Backtrace:
  1    SYSTEMPATH/CodeIgniter.php:894
       App\Controllers\Home()->index()

  2    SYSTEMPATH/CodeIgniter.php:467
       CodeIgniter\CodeIgniter()->runController(Object(App\Controllers\Home))

  3    SYSTEMPATH/CodeIgniter.php:350
       CodeIgniter\CodeIgniter()->handleRequest(null, Object(Config\Cache), false)

  4    FCPATH/index.php:40
       CodeIgniter\CodeIgniter()->run()

paulbalandan · 2022-03-29T08:11:32Z

In my understanding, discovery in Composer means scanning all known directories and files to Composer - not just limited to those that can be found in Commands/, Routes/, Filters/, etc.

If you do not want all of Composer’s known directories to be scanned

As such based on the above, discovery in Composer means registering all classes (whether or not those classes are CodeIgniter modules). If I'm not correct, then the implementing logic in Autoloader::discoverComposerNamespaces() is wrong as it registers ALL namespaces and classmaps found in Composer.

kenjis · 2022-03-29T09:02:50Z

@paulbalandan Okay, you think the error in #5834 (comment) is not a bug, and no problem.

And I don't agree with it.

@MGatner Can you give me your opinion?

MGatner · 2022-03-30T10:30:44Z

I think I'm not understanding something in these threads. Tell me what I have wrong here:

Composer's Autoloader should be registered once, always (if it is available) regardless of Config\Modules
Class discovery should work across all namespaces using our Autoloader
Module discovery should only control whether a component "goes looking" for more of its kind, e.g. if Events will check MyNamespace\Config\Events for a trigger, not whether I can access that class directly via autoloading

MGatner · 2022-03-30T10:34:01Z

Maybe Events was a bad example because they aren't actually classes. But take Services - I should be able to do this regardless of settings in Config\Modules:

\MyNamespace\Config\Services::foo()

But if discovery is disabled in Config\Modules then this should fail:

Config\Services::foo()

MGatner · 2022-03-30T10:34:53Z

Let me also add: from Paul's original description that it seems certain there are some things currently wrong with the implementation.

kenjis · 2022-03-30T12:18:01Z

@MGatner

Modules' "Auto-Discovery"($enabled) and "Auto-Discovery Within Composer Packages"($discoverInComposer) are different configs.

kenjis · 2022-04-02T04:18:26Z

I agree with 1 and 3.

Composer's Autoloader should be registered once, always (if it is available) regardless of Config\Modules

Class discovery should work across all namespaces using our Autoloader

Module discovery should only control whether a component "goes looking" for more of its kind, e.g. if Events will check MyNamespace\Config\Events for a trigger, not whether I can access that class directly via autoloading.

About 2.
Class discovery should work across all namespaces using our Autoloader or Composer autoloader.
Basically Composer autoloader loads Composer package files, our Autoloader loads Autoload::$psr4 namespaces.

kenjis · 2022-04-02T04:24:12Z

I sent another PR #5849.

kenjis · 2022-04-02T05:36:30Z

I have sent a PR to fix what appears to be a bug in Autoloader: #5850

MGatner · 2022-04-02T11:50:56Z

Discussion continued on the bugfix PR. @paulbalandan Can you weigh in there? I think deciding on that (whether it is indeed a bug) will clarify the rest of these.

kenjis · 2022-04-05T08:07:56Z

#5850 was merged. So close this.

paulbalandan added 2 commits March 27, 2022 01:14

Unregister Composer's autoloader from the stack

ef7dec6

Simplify code

4c58242

samsonasik approved these changes Mar 27, 2022

View reviewed changes

kenjis reviewed Mar 27, 2022

View reviewed changes

Remove double call to COMPOSER_PATH in bootstrap

85f7e69

paulbalandan force-pushed the fix-overlapping-autoloader branch from 136903c to 85f7e69 Compare March 28, 2022 14:09

kenjis mentioned this pull request Apr 2, 2022

fix: Commands::discoverCommands() loads incorrect classname #5849

Merged

5 tasks

kenjis mentioned this pull request Apr 2, 2022

fix: [Autoloader] Composer classmap usage #5850

Merged

5 tasks

kenjis closed this Apr 5, 2022

paulbalandan deleted the fix-overlapping-autoloader branch April 17, 2022 12:46

Uh oh!

Conversation

paulbalandan commented Mar 26, 2022

Uh oh!

kenjis Mar 27, 2022

Choose a reason for hiding this comment

Uh oh!

paulbalandan Mar 28, 2022

Choose a reason for hiding this comment

Uh oh!

kenjis Mar 28, 2022

Choose a reason for hiding this comment

Uh oh!

paulbalandan Mar 28, 2022

Choose a reason for hiding this comment

Uh oh!

paulbalandan Mar 28, 2022

Choose a reason for hiding this comment

Uh oh!

paulbalandan Mar 28, 2022

Choose a reason for hiding this comment

Uh oh!

kenjis commented Mar 27, 2022

Uh oh!

kenjis commented Mar 27, 2022

Uh oh!

MGatner commented Mar 27, 2022

Uh oh!

paulbalandan commented Mar 28, 2022

Uh oh!

kenjis commented Mar 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paulbalandan commented Mar 29, 2022

Uh oh!

kenjis commented Mar 29, 2022

Uh oh!

MGatner commented Mar 30, 2022

Uh oh!

MGatner commented Mar 30, 2022

Uh oh!

MGatner commented Mar 30, 2022

Uh oh!

kenjis commented Mar 30, 2022

Uh oh!

kenjis commented Apr 2, 2022

Uh oh!

kenjis commented Apr 2, 2022

Uh oh!

kenjis commented Apr 2, 2022

Uh oh!

MGatner commented Apr 2, 2022

Uh oh!

kenjis commented Apr 5, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kenjis commented Mar 28, 2022 •

edited

Loading