1up-lab/OneupUploaderBundle

ResponseInterface should implement \ArrayAccess

ndench opened this issue · 4 comments

Bug Report

Q A
BC Break no
Version 3.0

Summary

According to the documentation, you can process uploaded files using custom logic and pass update the Response using array access to pass extra data through:

// ...

class UploadListener
{
    // ...
    
    public function onUpload(PostPersistEvent $event)
    {
        //...
        
        //if everything went fine
        $response = $event->getResponse();
        $response['success'] = true;
        return $response;
    }
}

The issue here is:

  • $event->getResponse() returns ResponseInterface
  • ResponseInterface does not implement \ArrayAccess
  • So at "compile time" (eg. static analysis with phpstan/psalm/your IDE), we receive an error with this approach.
  • It works at runtime, but only because all implementations of ResponseInterface extend AbstractResponse, which implements \ArrayAccess

How to reproduce

  1. Create an UploadListener as described in the documentation
  2. Use some form of static analysis, eg. psalm, phpstan, or you IDE of choice.
  3. Notice the reported error

PhpStan:
image

Psalm:
image

PhpStorm with EA Extended plugin:
image

Suggested fix

Move the implements \ArrayAccess declaration from AbstractResponse to ResponseInterface.

Happy to send through a PR for this, just want to validate that there isn't something I'm missing.

Hey @ndench

Thanks for the issue! Seems totally legit and reasonable to me.
IMHO is altering an interface a BC break though, isn't it?

@bytehead that's a good point about BC. We should be able to use @implements in phpdoc to be compatible. Then switch it to a proper implements in the next major version.

Unfortunately, @implements and @extends didn't work. But I have created a PR for the next major version.

Update: Apparently we can use @mixin instead. I've opened #392 to use this and not break BC.

Closing this in favor of #392 and #391.