Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cannot use typed properties in queued listeners #781

Closed
golonix opened this issue Nov 29, 2019 · 6 comments
Closed

Cannot use typed properties in queued listeners #781

golonix opened this issue Nov 29, 2019 · 6 comments
Labels

Comments

@golonix
Copy link

golonix commented Nov 29, 2019

  • Telescope Version: v2.1
  • Laravel Version: 6.6.0
  • PHP Version: 7.4.0
  • Database Driver & Version: irrelevant

Description:

When using a queued listener, it is impossible to have typed properties declared in that listener's class.

class SomeListener implements ShouldQueue
{
    use InteractsWithQueue;

    /**
     * @var SubscriptionService
     */
    private SubscriptionService $subscriptionService;

    /**
     * @param SubscriptionService $subscriptionService
     */
    public function __construct(SubscriptionService $subscriptionService)
    {
        $this->subscriptionService = $subscriptionService;
    }

    public function handle(...)
    {
    }
}

When Telescope is enabled code above will produce an error:

Typed property App\Listeners\SomeListener::$subscriptionService must not be accessed before initialization

Steps To Reproduce:

Create any queued listener and enable Telescope.

Issue is caused by ExtractTags::modelsFor() method:

    protected static function modelsFor(array $targets)
    {
        $models = [];

        foreach ($targets as $target) {
            $models[] = collect((new ReflectionClass($target))->getProperties())->map(function ($property) use ($target) {
                $property->setAccessible(true);

                $value = $property->getValue($target); // <-- This throws error

                if ($value instanceof Model) {
                    return [$value];
                } elseif ($value instanceof EloquentCollection) {
                    return $value->flatten();
                }
            })->collapse()->filter()->all();
        }

        return collect(Arr::collapse($models))->unique();
    }

There are possible workarounds:

  • do not use typed properties
  • always initiate typed property (defacto allow nullable values and set null as default).

IMHO developer should not be forced to either not use typed properties or allow null values in properties.

I've got very simple fix for that, but not sure if that is sufficient (probably not). Simply, there should be check if a property has been initialized before it can be accessed (https://wiki.php.net/rfc/typed_properties_v2#reflection):

    protected static function modelsFor(array $targets)
    {
        $models = [];

        foreach ($targets as $target) {
            $models[] = collect((new ReflectionClass($target))->getProperties())->map(function ($property) use ($target) {
                $property->setAccessible(true);

                // Check if property is initialized (for PHP >= 7.4.0)
                if (PHP_VERSION_ID >= 70400 && is_object($target) && !$property->isInitialized($target)) {
                    return ;
                }

                $value = $property->getValue($target);

                if ($value instanceof Model) {
                    return [$value];
                } elseif ($value instanceof EloquentCollection) {
                    return $value->flatten();
                }
            })->collapse()->filter()->all();
        }

        return collect(Arr::collapse($models))->unique();
    }

If proposed solution is acceptable, I can create MR for that.

@driesvints driesvints changed the title [PHP 7.4] Cannot use typed properties in queued listeners Cannot use typed properties in queued listeners Dec 9, 2019
@driesvints
Copy link
Member

We already solved typed properties in laravel/framework with this PR: laravel/framework#30605

Maybe some ideas from it can be reused here.

@driesvints driesvints added the bug label Dec 9, 2019
@vigneshgurusamy
Copy link

@driesvints Any update on this issue?

@driesvints
Copy link
Member

Ping @themsaid

@mengidd
Copy link

mengidd commented Mar 13, 2020

Any update on this? 🙂

@lucasmichot
Copy link
Contributor

I've made a PR for that - see #879

@driesvints
Copy link
Member

This was fixed and will be in the next release. Thanks @lucasmichot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants