alekitto / class-finder

Utility classes to help discover other classes/namespaces
https://alekitto.github.io/class-finder/
MIT License
27 stars 6 forks source link

Error handler has changed, cannot unregister the handler #11

Closed cnizzardini closed 7 months ago

cnizzardini commented 8 months ago

I am encountering this exception in version kcs/class-finder 0.3.2 and I don't really understand it. I am on PHP 8.1. Can you assist?

The code I am ultimately executing is this:

// $namespace = 'App\Event';
$finder = (new ComposerFinder())->inNamespace($namespace);

Error in: ROOT/vendor/kcs/class-finder/lib/Util/ErrorHandler.php, line 72

            $previous = set_error_handler(static fn () => false);
            restore_error_handler();
            if ($previous !== [self::class, 'handleError']) {
                throw new Error('Error handler has changed, cannot unregister the handler'); // @phpstan-ignore-line
            }

            restore_error_handler();
            self::$registered = false;
cnizzardini commented 8 months ago

Okay, I debugged this more. I think its because of how I am loading in my packages via composer.json:

    "repositories": [
        {
            "type": "path",
            "url": "../dev/plugins/bake",
            "options": {
                "symlink": true
            }
        },

Why? Because I am importing some code into a demo project for a package I maintain via docker-compose and this is the best way I could figure to do it:

    volumes:
      - ./app:/srv/app
      - ~/dev:/srv/dev

Luckily, this is not a critical feature for me to test, but I am wondering why this doesn't work... There is a bit more I could explain here, but I am wondering if perhaps you can spot the problem this causes with your library?

alekitto commented 8 months ago

Is your code registering an error handler (calling set_error_handler)?

cnizzardini commented 8 months ago

Yes indirectly via the CakePHP framework: https://github.com/cakephp/cakephp/blob/5.x/src/Error/ErrorTrap.php#L93

cnizzardini commented 8 months ago

I dug into this more because I like this library and don't have great alternatives. I use it widely in packages I maintain. The issue is it's loading in my tests/bootstrap.php file which was also including my applications bootstrap.php file. This explains away various warnings I was receiving as well.

There are a few ways I validated this.

  1. Removing this line in my composer.json: "App\\Test\\": "tests/", which is obviously not a viable solution.
  2. Removing this line in my tests/bootstrap: require dirname(__DIR__) . '/config/bootstrap.php';

The question I have is, why is this library loading in a non-PSR-4 file like bootstrap and is there a way to prevent this behavior?

cnizzardini commented 8 months ago

This seems like what I need.

$finder = (new Psr4Finder($namespace, __DIR__));

Not sure I understood the docs or if the docs explain this well..

alekitto commented 8 months ago

I think I understood what is causing this issue. Probably I should unwind the error handler stack and remove only the handler I've registered but I don't really know if it's a viable solution in every case. I'll try this in the next days, I'll ping you once it's done.

However, to better explain how it's working: your bootstrap.php is included because is placed inside a folder containing a Psr4 structure, but without including it, the finder cannot know if it's containing a valid class or not.

Moving the bootstrap.php outside the tests/ folder will fix this. Another solution is to use an optimized autoloader (use -o flag when doing a composer install) as it will generate a full class map of your classes and will cause this library not to recursively iterate the folders.

Additionally, I'm currently working on adding the modifications of #10 to all the finders, so you will be able to call fileFilter on the finder and simply ignore the bootstrap.php file when encountered (I'll release this tomorrow).

alekitto commented 8 months ago

Anyway, what you're doing here:

$finder = (new Psr4Finder($namespace, __DIR__));

should be the same as

$finder = new ComposerFinder();
$finder
  ->inNamespace([$namespace])
  ->in([__DIR__]);

The in filter should prevent the inclusion of unwanted files.

Probably the doc is not enough clear on this, I think I could expand this a little more...

alekitto commented 8 months ago

ErrorHandler now unwinds the error handlers stack to remove itself and leave the other handlers intact. @cnizzardini can you try the dev-master version and tell me if it's working?

cnizzardini commented 8 months ago

It was going to require a bit of work to try that out for me. Instead I directly modified the vendor source with your changes to ErrorHandler. This worked. I also realized the easier solution is simply modifying my test bootstrap to require the main config once:

require_once dirname(__DIR__) . '/config/bootstrap.php';

^ Why PHP 101 didn't occur to me initially is anyones guess.

alekitto commented 8 months ago

I still don't believe this library should load non-class/interface files as it is titled "class-finder"

I know, but the library cannot know if a file contains a class/interface/trait before loading it 😅

cnizzardini commented 8 months ago

Yes I understand. Feel free to close this issue.