[Tags] Cannot use use model-less ::class gate on discussion and tags when tags extensions is enabled

[clarkwinkelmann]

Bug Report

Current Behavior
Extensions that use model-less abilities on the Discuss and Tag policies fail when the Tags extension is enabled.

The ability to use model-less ::class abilities on policies was added in flarum/framework#1977 . It's correctly unit-tested in https://github.com/flarum/core/blob/master/tests/unit/User/AbstractPolicyTest.php but there are no unit tests for each policy specifically, and no tests with core extensions enabled.

Steps to Reproduce

Unit tests showing the issue.

Works:

<?php

namespace Tests\integration;

use Flarum\Discussion\Discussion;
use Flarum\Extend\Policy;
use Flarum\User\Access\AbstractPolicy;
use Flarum\User\User;

class ModelLessPolicyTest extends \Flarum\Testing\integration\TestCase
{
    protected function setUp(): void
    {
        parent::setUp();

        $this->extend((new Policy())->modelPolicy(Discussion::class, DiscussionPolicy::class));
    }

    /**
     * @test
     */
    public function callPolicy()
    {
        $this->app();

        $user = User::findOrFail(1);

        $this->assertTrue($user->can('createSpecial', Discussion::class));
    }
}

class DiscussionPolicy extends AbstractPolicy
{
    public function createSpecial(User $actor)
    {
        return $this->allow();
    }
}

Doesn't work (added extension tags):

<?php

namespace Tests\integration;

use Flarum\Discussion\Discussion;
use Flarum\Extend\Policy;
use Flarum\User\Access\AbstractPolicy;
use Flarum\User\User;

class ModelLessPolicyTest extends \Flarum\Testing\integration\TestCase
{
    protected function setUp(): void
    {
        parent::setUp();

        $this->extension('flarum-tags');

        $this->extend((new Policy())->modelPolicy(Discussion::class, DiscussionPolicy::class));
    }

    /**
     * @test
     */
    public function callPolicy()
    {
        $this->app();

        $user = User::findOrFail(1);

        $this->assertTrue($user->can('createSpecial', Discussion::class));
    }
}

class DiscussionPolicy extends AbstractPolicy
{
    public function createSpecial(User $actor)
    {
        return $this->allow();
    }
}

Error:

There was 1 error:

1) Tests\integration\ModelLessPolicyTest::callPolicy
TypeError: Argument 3 passed to Flarum\Tags\Access\DiscussionPolicy::can() must be an instance of Flarum\Discussion\Discussion, string given

/[...]/vendor/flarum/tags/src/Access/DiscussionPolicy.php:39
/[...]/vendor/flarum/core/src/User/Access/AbstractPolicy.php:61
/[...]/vendor/flarum/core/src/User/Access/Gate.php:78
/[...]/vendor/flarum/core/src/User/User.php:775
/[...]/tests/integration/ModelLessPolicyTest.php:30

Expected Behavior
A clear and concise description of what you expected to happen.

Screenshots
If applicable, add screenshots to help explain your problem.

Environment

• Flarum version: 1.0.1
• Hosting environment: local
• PHP version: 7.4

Possible Solution

The problem is that the third parameter to DiscussionPolicy::can was type-hinted as Discussion in the Tags extension when it will be string for any call to a model-less ability:

https://github.com/flarum/tags/blob/453169d8093feb1ad523cfdbdcf47c3b3a391cab/src/Access/DiscussionPolicy.php#L39

The tag policy seems to be affected by the same issue:

https://github.com/flarum/tags/blob/453169d8093feb1ad523cfdbdcf47c3b3a391cab/src/Access/TagPolicy.php#L18

The PHP type-hint should probably be removed. The PHPdoc could be updated to Discussion|string (respectively, Tag|string). Then we should probably return early if the third parameter isn't a model, something like if (!($discussion instanceof Discussion)) { return; }

Additional Context
As a workaround I have passed new Discussion to the gate but that's not ideal since ::class is supposed to be supported.

I have not checked if other core extensions are impacted by this.

This is not a regression, it seems to have been broken since the start. The Discussion type-hint on the third parameter has been there for years. There must just have been no-one trying to use a ::class ability on the Discussion policy yet.

Another reason this might not have manifested earlier is because of the changes to the policies we did a few versions ago. Previously the can() method(s) would not have been called if an ability method existed with that name. But now I think we always call all the ability and all the can methods and then compute the value based on all responses.