PHPStan lvl 4 & Fix/issue 233 followup

[SanderMuller]

@staudenmeir can you look at the 3 remaining phpstan errors? PHPStan is saying that the values cannot be null in these cases, and I would agree from a theoretical look at it, but maybe you understand the cases / code flows where they can be null

[staudenmeir]

can you look at the 3 remaining phpstan errors? PHPStan is saying that the values cannot be null in these cases, and I would agree from a theoretical look at it, but maybe you understand the cases / code flows where they can be null

The keys array contains null when you load ancestors or siblings of root nodes (where parent_id is null).

I overrode the type of $keys with a @var comment. Technically, the return type of Laravel's getKeys() is incorrect, as the array can also include null for native relationships. Maybe I'll submit a PR.

[SanderMuller]

can you look at the 3 remaining phpstan errors? PHPStan is saying that the values cannot be null in these cases, and I would agree from a theoretical look at it, but maybe you understand the cases / code flows where they can be null

The keys array contains null when you load ancestors or siblings of root nodes (where parent_id is null).

I overrode the type of $keys with a @var comment. Technically, the return type of Laravel's getKeys() is incorrect, as the array can also include null for native relationships. Maybe I'll submit a PR.

Makes sense, thanks for explaining! Maybe someday Laravel will switch to native types, solving these docblock inconsistencies.

Do you have an idea why the Singlestore CI is failing?

[staudenmeir]

The SingleStore issues are coming from this change: https://github.com/staudenmeir/laravel-adjacency-list/pull/257/files#diff-a5a1ae0a346b903a22c8c59bd60526cb75026f9f0f2b70d82f60315758198fe4

The SingleStoreConnection class also extends from MySqlConnection and so the SingleStoreGrammar doesn't get used anymore.

This works, but isn't very elegant...

    public function getExpressionGrammar()
    {
        /** @var \Illuminate\Database\Connection $connection */
        $connection = $this->query->getConnection();

        switch ($connection->getDriverName()) {
            case 'mysql':
                /** @var \Illuminate\Database\MySqlConnection $connection */
                $grammar = $connection->isMaria()
                    ? new MariaDbGrammar($this->model)
                    : new MySqlGrammar($this->model);

                $grammar = $connection->withTablePrefix($grammar);

                break;
            case 'mariadb':
                $grammar = $connection->withTablePrefix(
                    new MariaDbGrammar($this->model)
                );

                break;
            case 'pgsql':
                $grammar = $connection->withTablePrefix(
                    new PostgresGrammar($this->model)
                );

                break;
            case 'sqlite':
                $grammar = $connection->withTablePrefix(
                    new SQLiteGrammar($this->model)
                );

                break;
            case 'sqlsrv':
                $grammar = $connection->withTablePrefix(
                    new SqlServerGrammar($this->model)
                );

                break;
            case 'singlestore':
                $grammar = $connection->withTablePrefix(
                    new SingleStoreGrammar($this->model)
                );

                break;
            case 'firebird':
                $grammar = $connection->withTablePrefix(
                    new FirebirdGrammar($this->model)
                );

                break;
            default:
                throw new RuntimeException('This database is not supported.'); // @codeCoverageIgnore
        }

        /** @var \Staudenmeir\LaravelAdjacencyList\Query\Grammars\ExpressionGrammar $grammar */
        return $grammar;
    }

[SanderMuller]

@staudenmeir I've resolved it with this 63749db69179808974506b1216112e229719805c, what do you think?

[staudenmeir]

@SanderMuller Thanks. Unfortunately, this behaves differently from the current implementation. We need to keep using $connection->isMaria() because the case $connection->getDriverName() === 'mysql' && $connection instanceof MariaDbConnection can't occur.

There are two ways of using Laravel with MariaDB:

• With the mysql driver: Here, Laravel and the package use $connection->isMaria() to detect MariaDB by checking the actual server. This is the case that wouldn't work anymore.
• With the (rather new) mariadb driver. The MariaDbConnection class is only used in this case.

[SanderMuller]

@staudenmeir let me know what you think about 8fe6aaef74cba956659e4c3428d846cb808e14f9

[staudenmeir]

Perfect 👍

[SanderMuller]

@staudenmeir let me know if this PR still requires some changes or elaboration

[SanderMuller]

@calebdw similar PR here if you are in a review mood 🙏

[calebdw]

All of the relation generics are going to need to be updated:

[SanderMuller]

All of the relation generics are going to need to be updated:

I think this would require your fix you mentioned on the other PR, because:

 Line   src/Eloquent/Relations/BelongsToManyOfDescendants.php                                       
 ------ -------------------------------------------------------------------------------------------- 
  16     Generic type                                                                                
         Illuminate\Database\Eloquent\Relations\BelongsToMany<Illuminate\Database\Eloquent\Model,    
         Illuminate\Database\Eloquent\Model> in PHPDoc tag @extends specifies 2 template types, but  
         class Illuminate\Database\Eloquent\Relations\BelongsToMany supports only 1: TRelatedModel   
         🪪  generics.moreTypes  

[SanderMuller]

@calebdw & @staudenmeir I've applied all feedback I managed to fix. There's a couple of @phpstan-ignore statements that would ideally be removed, but they're related to generics in extended relations. Could use help on that, or we move to accept this progress for now

[SanderMuller]

Looks pretty good---great work!

One thing to keep in mind is that the Relations are going to be disjunctive between L10 and L11.

When using this package with L11 the relations should use the new generic types introduced in the framework, but of course they don't exist on older versions

Thanks! What would be the best way in dealing with this? Would be a shame to make a major release just for this.

[calebdw]

What would be the best way in dealing with this? Would be a shame to make a major release just for this.

That's probably the easiest thing to do, you can also drop support for lower version while you're at it.

Another thing that could be done is creating a versioned stub system like what larastan does for the different versions