Skip to content

Automatically resolve the flag's type if it's a class string#146

Merged
taylorotwell merged 1 commit intolaravel:1.xfrom
RVxLab:1.x
Aug 5, 2025
Merged

Automatically resolve the flag's type if it's a class string#146
taylorotwell merged 1 commit intolaravel:1.xfrom
RVxLab:1.x

Conversation

@RVxLab
Copy link
Contributor

@RVxLab RVxLab commented Aug 5, 2025

This PR extends the docblock of the instance method to automatically resolve the feature flag's class if it's a class string.

@taylorotwell taylorotwell merged commit 0b7b3b6 into laravel:1.x Aug 5, 2025
10 checks passed
@timacdonald
Copy link
Member

timacdonald commented Aug 18, 2025

@RVxLab, this does not seem to work. I'm opening a PR to revert the change but would love to know if you tested this and I'm doing something wrong before we merge it.

I've tested via PHPStan and it never seems to return the class instance. Always mixed. See below for the test:

<?php

class MyFeatureClass
{
    //
}

/** @var Laravel\Pennant\Drivers\Decorator $feature */

// ... //

$var = $feature->instance('foo');
PHPStan\Testing\assertType('mixed', $var);

$var = $feature->instance('NonExistentClass');
PHPStan\Testing\assertType('mixed', $var);

$var = $feature->instance(MyFeatureClass::class);
PHPStan\Testing\assertType('MyFeatureClass', $var);
Screenshot 2025-08-18 at 12 03 21

@RVxLab
Copy link
Contributor Author

RVxLab commented Aug 18, 2025

Hi @timacdonald,

I tested this end to end in PHPStorm and the typings looked mostly correct. I wasn't even aware PHPStan had functions like this.

The way I tested it was by creating a blank Laravel project and adding Pennant through a local repository. Then I created a class-based flag with a method in it:

<?php

namespace App\Features;

use Illuminate\Support\Lottery;

class MyFeatureClass
{
    /**
     * Resolve the feature's initial value.
     */
    public function resolve(mixed $scope): mixed
    {
        return false;
    }

    public function doSomething(): void
    {
        //
    }
}

PhpStorm was able to pick that method up correctly.

image image

Doing this with a string gives the expected behaviour as well.

image

When I use your testing method I get the same result as you did and I have no clue why since the parameter is a class-string. Calling it via the facade also errors, but differently.

 ------ ----------------------------------------------------------------------------------------------------- 
  Line   web.php                                                                                              
 ------ ----------------------------------------------------------------------------------------------------- 
  10     Parameter #1 $name of static method Laravel\Pennant\Feature::instance() expects void, string given.  
         🪪  argument.type                                                                                    
  10     Result of static method Laravel\Pennant\Feature::instance() (void) is used.                          
         🪪  staticMethod.void                                                                                
  12     Parameter #1 $name of static method Laravel\Pennant\Feature::instance() expects void, string given.  
         🪪  argument.type                                                                                    
  12     Result of static method Laravel\Pennant\Feature::instance() (void) is used.                          
         🪪  staticMethod.void                                                                                
  13     Cannot call method doSomething() on null.                                                            
         🪪  method.nonObject                                                                                 
  15     Expected type mixed, actual: null                                                                    
  16     Expected type App\Features\MyFeatureClass, actual: null                                              
  26     Cannot call method doSomething() on mixed.                                                           
         🪪  method.nonObject                                                                                 
  29     Expected type App\Features\MyFeatureClass, actual: mixed                                             
 ------ ----------------------------------------------------------------------------------------------------- 

Removing the TFlag from class-string<TFlag> produces a different error.

Expected type App\Features\MyFeatureClass, actual: 'App\\Features\\MyFeatureClass'

I really can't explain this, sadly.

Probably do that revert and I'll try again another time.

@timacdonald
Copy link
Member

No worries. Thanks for trying. I did try and get it working myself, but had no luck.

@maartenpaauw
Copy link
Contributor

maartenpaauw commented Aug 18, 2025

@timacdonald Just for a bit of guidance; the void errors above in @RVxLab PHPStan logs are because of the facade docblock were incorrect. I've fixed that in the following PR (not released yet): #147

@RVxLab
Copy link
Contributor Author

RVxLab commented Aug 18, 2025

@timacdonald Just for a bit of guidance; the void errors above in @RVxLab PHPStan logs are because of the facade docblock were incorrect. I've fixed that in the following PR: #147

I was about to make a reply about that, though it got reverted in b0ef334 through a Github Action.

https://github.com/laravel/pennant/blob/1.x/.github/workflows/facade.yml

@timacdonald
Copy link
Member

timacdonald commented Aug 18, 2025

@maartenpaauw, my test above is calling the Laravel\Pennant\Drivers\Decorator::instance method directly, not via the Facade.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants