chore: Setup PHPStan Level 5

[SychO9]

Closes #3559

Changes proposed in this pull request:

• Sets up PHPStan infrastructure.
• Adapts Docblocks and certain backend typings.
• Fixes some issues caught by phpstan.

Reviewers should focus on:

• Pay most attention to codebase edits.
• Generally they are just minor docblock improvements.
• The most notable change is the replacement of \Illuminate\Database\ConnectionInterface by \Illuminate\Database\Connection. Ideally we would stick to injecting the interface as that is more correct to separate the implementation from the contract. But in most of our codebase, we use methods that only exist within the implementation and not the interface. We could just leave it and fool phpstan with docblocks, but I thought it would be better to be explicit about what we expect injected. Thoughts are very welcome about this especially.

Necessity

• Has the problem that is being solved here been clearly explained?
• If applicable, have various options for solving this problem been considered?
• For core PRs, does this need to be in core, or could it be in an extension?
• Are we willing to maintain this for years / potentially forever?

Confirmed

• Frontend changes: tested on a local Flarum installation.
• Backend changes: tests are green (run composer test).
• Core developer confirmed locally this works as intended.
• Tests have been added, or are not appropriate here.

[luceos]

Okay this was a big one.

Most of this looks great, a few things came up that we need to take a decision on:

• I prefer not switching from the DatabaseInterface to the Database abstract. The interface is guaranteed to be bound into ioc, that's the whole idea of the interface. Using the implementation everywhere seems off to me. If it really helps we can typehint the property with that class as well DatabaseInterface|Connection, but dropping the interface - especially in construct arguments typehints - is going to bite us in the long run.
• I noticed some places where full class names were used and not imported, let's try to import all the things.
• Some changes to go from filesystem Cloud interface to Filesystem aren't correct. Only the Cloud interface has the url method we need. We cannot drop that or we'll break things.
• I saw that we are doing exists and get or delete sometimes. If possible it would be a huge deal in cutting latency if we can use only get and delete and capture any thrown errors to reduce api calls to cloud based storage.
• In some changes we moved from self to static. Now I can be wrong, but I thought that doing that enforces the returned class to be this implementation and not any class extending this implementation. This would be a huge deal in extensibility and being able to apply raw/higher level overrides.

[SychO9]

(@luceos gonna reply here globally here about some of the issues raised here)

Database connection interface
that's the thing with ConnectionInterface, I dislike having to inject the actual implementation, but we either do that because that's what our code actually expects because it uses methods exclusive to the implementation or we hack around it.

// The interface
interface ConnectionInterface { ... }

// Illuminate implementation
abstract class Connection extends ConnectionInterface
{
    public function getSchemaBuilder(); // doesn't exist in the interface
}

// Solution 1: actually inject the implementation because that's what we actually expect.
class Example
{
    public function __construct(Connection $db) { ... }

    public function example()
    {
        $this->db->getSchemaBuilder()->...
    }
}

// Solution 2: hack around it everywhere we expect implementation-specific instances.
class Example
{
    public function __construct(ConnetionInterface $db) { ... }

    public function example()
    {
        /** @var Connection */
        $db = $this->db;

        $db->getSchemaBuilder()->...
    }
}

---

Filesystem vs Cloud (#3553 (comment), #3553 (comment))
Illuminate's filesystem Factory contract disk method returns a Filesystem contract, not Cloud. And since PHP doesn't have generics, we're gonna have to hack it with a docblock one way or the other.

---

self vs static
PHPStan complains about using static as it can be unsafe when extending and lead to fatal errors. I'm personally not a fan of using static unless it really makes sense. Otherwise, we could add a global ignore for phpstan's error about the static keyword.

---

exists() vs get()
I understand the mentioned performance gain, but I believe that is outside the scope of this PR. We only change has to exists and others because the older methods are deprecated.

[tankerkiller125]

@SychO9 in regards to generics PHPStan has a way to work around that issue https://phpstan.org/blog/generics-in-php-using-phpdocs

NVM, I just saw your mention of the docblock 🤦

[luceos]

Thanks for the clarification. I hope this doesn't bite is in the behind in the long run 💪

[SychO9]

I do think it might be better to use stubs to tell phpstan that the interface does have the methods even though that's not the case.

Same for the filesystem, perhaps using a stub to change the return type to Cloud might be better.

Not sure yet, need to consider this more.

[SychO9]

New changes:

• Reverts to injecting ConnectionInterface instead of Connection and ignores related errors, while our codebase expects the actual implementation and it would make sense to inject it instead. I feel like it would be more sane to keep the interface instead.
• Uses a stub to modify the Filesystem contract to have the disk return type changed to Cloud. That's what our codebase expects everywhere.
• Also revets static to self changes and ignores errors. I think this needs more discussion in the long run, while we do this for the sake of extensibility, it just seems to be used anywhere and everywhere, even in private API.

[luceos]

• Also revets static to self changes and ignores errors. I think this needs more discussion in the long run, while we do this for the sake of extensibility, it just seems to be used anywhere and everywhere, even in private API.

You mean you reverted self to static?

I'm okay with this if that's the case 👍 Thanks for considering my argumentation.

[SychO9]

yup self to staticmy bad

[askvortsov1]

re: the exists + get into one issue: if we want to do this, we should consider making either a wrapper interface/class or an explicit util method. Probably the latter.

[askvortsov1]

Another general comment. From a maintainer's / reviewer's perspective, it can be hard to tell why certain things are overriden. I think it would be helpful in the future if there were comments on each stub file explaining what the goal of overriding / stubbing the class in question is.

[askvortsov1]