Determine tmpDir path relative to config file when appropriate

[mattwellss]

Fix #161

[shochdoerfer]

Very much appreciated. Thanks a lot for improving this PHPStan extension!

[shochdoerfer]

We should add a call to \Nette\DI\Helpers::expand() in the same way as PHPstan does it to make sure we are dealing with the same configuration: https://github.com/phpstan/phpstan-src/blob/master/src/Command/CommandHelper.php#L149

[mattwellss]

I think in that case, we'll need to add nette/di as a dependency (probably not a big deal). The class Nette/DI/Helpers is marked @internal, which my IDE treats like sorta like a deprecated class.

Given those points, are you comfortable using that method? I'd like to follow your lead on decisions like this.

[shochdoerfer]

I feel we might not need this bock and rather add the same !isset() check as PHPStan uses: https://github.com/phpstan/phpstan-src/blob/master/src/Command/CommandHelper.php#L228-L231

[mattwellss]

Is it this check that we should replace with the simpler isset: https://github.com/bitExpert/phpstan-magento/pull/163/files/137b8dfd770612cc9cc057d6aaf537cfb650dffc#diff-f79e1f84a3df304b5be8b1bd2f1a36d4bb8417d91e8ae7144e648379d697a57eL48?

If not, I think I need some clarification regarding the change you'd like to see.

[shochdoerfer]

@mattwellss sorry for not getting back to you earlier. I am still super busy with my work, this should hopefully get better next week. I had the chance to discuss things with Ondřej. He pointed out that the bootstrap files get a $container instance injected from which we can pull the dependencies instead of creating them on our own. Additionally, that means, we can omit the whole config parsing logic.

See here how the bootstrap files are invoked: https://github.com/phpstan/phpstan-src/blob/master/src/Command/CommandHelper.php#L475

If you find some time to refactor the logic, feel free. Otherwise, I'll do it next week or the week after. My goal is to try and get the next extension release done before Christmas with both of your PR's included.

[mattwellss]

@shochdoerfer wow, the autoloader is so much cleaner using the $container! What an improvement, thanks for finding that.

Does this change mean we can eliminate the nette/neon dependency from composer.json?

[shochdoerfer]

@mattwellss agree, looks so much better. Yes, we don't need the nette/neon dependency anymore. Good catch.

Also, we can improve things a bit more ;) We can define the autoloader classes as services in the extension.neon file and then pull them from the DI container.

As it looks to me, we can declare the services in the extensions.neon file this:

mockAutoloader :
		class: bitExpert\PHPStan\Magento\Autoload\MockAutoloader

and then pull it from the container like this:

$mockAutoloader= $container->getService('mockAutoloader');

Since the require_once() in PHPStand is embedded in a closure already, we could also get rid of this I think:

(function (Container $container) {
})($container);

[mattwellss]

OK, I got a little wild with the latest commit, happy to roll back some of my decisions if needed. 😄

The goal with my latest changes is to allow users of the extension to define their own autoloaders as needed. This results in a lot of extra configuration in extension.neon, and might be totally unnecessary, if this extension is intended to cover all potential autoloading needs.

Curious to hear your thoughts.

[shochdoerfer]

I like your way of thinking but we should adjust things a little bit ;)

1. I feel it would be better to introduce an interface for the autoloader classes and omit the Magento autoloaders config section in extension.neon. The interface could be defined as:

interface Autoloader
{
    public function autoload(string $class): void;

    public function canThrow(): bool;
    
    public function canPrepend(): bool;
}

2. In extension.neon add a tag for the autoloader services like phpstan.magento.autoloader. Then in autoload.php you could query all tagged autoloader services via $container->getServicesByTag('phpstan.magento.autoloader').

3. Since we have the interface as defined above, you can get the autoloader configuration by querying the respective methods:

\spl_autoload_register([$autoloader, $method], $autoloader->canThrow(), $autoloader->canPrepend());

What do you think?

Is the FileCacheStorage directory correctly configured? You set it to %tmpDir%, in the PHPStan config it is set to %tmpDir%/cache/PHPStan (and I think I used that path in the old autoload.php implementation). Or am I missing something?
https://github.com/phpstan/phpstan-src/blob/master/conf/config.neon#L1560

[mattwellss]

Good suggestions re: defining autoloaders! I'll work on that.

Oops, you're totally right about the $directory argument for cache storage... I'll update it. Do you know if we can extract the value configured by PHPStan's core?

[shochdoerfer]

I don't think we can pull the value from the DI container, because it's defined inline. Leave it as it is, for now, that's fine for me. I will try to figure out what was the reason for creating the FileCacheStorage class. Maybe we can drop that and depend on PHPStan's storage class. Then we could avoid the problem of having 2 cache directory definitions.

Maybe we should add one minor thing to the autoload.php file: And instanceof check in the foreach loop would be nice. Just to avoid that someone adds the tag to a different class.

[mattwellss]

I will try to figure out what was the reason for creating the FileCacheStorage class.

PHPStan's built-in FileCacheStorage returns the contents of the cached item, but we want the path of the cached item. I don't imagine we want to start using eval, but I suppose that approach would allow us to use the built-in cache.

I'll add a type check for the autoloader instance

[shochdoerfer]

@mattwellss the final PHPStan complaint needs to be fixed like this: https://github.com/bitExpert/phpstan-magento/blob/master/phpstan.neon#L11

Thanks for doing the annoying work ;)

[mattwellss]

Neat! I don't mind adding types and such. That's just PHPStan forcing us to be careful

[shochdoerfer]

And merged ;)

[mattwellss]

@shochdoerfer I forgot to remove nette/neon 😖 sorry!

[shochdoerfer]

No worries, I have it on my list to fix ;)