IDE support for `Filesystem\Asset` proxied methods

gaufde commented 1 month ago

Description

Code like asset('logo.svg')->read() is flagged in PhpStorm because "Method 'read' not found in Kirby\Filesystem\Asset".

Expected behavior
No warning to appear for valid code.

Screenshots
None

To reproduce

Use an IDE with good code "understanding" like PhpStorm (or probably VS Code with Intelephense) that is configured to inspect for missing methods.
Paste asset('logo.svg')->read() into a template
The error should appear automatically

Your setup

Kirby Version
4.3.0 and 5.0.0-alpha.1

Your system (please complete the following information)

Device: MacBook Pro M1 Max
MacOS: 13.6.7
PhpStorm 2024.1.4

After talking to Lukas Kleinschmidt about whether his typing plugin could handle this, it seems like it would be best if Kirby core could add some @mixin annotations directly to fix this issue. See: https://github.com/lukaskleinschmidt/kirby-types/issues/4#issuecomment-2264734449

Lukas mentioned I could try to submit a PR for this, but the relevant source code wasn't entirely clear to me. It seems like the Kirby\Filesystem\IsFile trait can return either a Kirby\Filesystem\File or Kirby\Image\Image object, which I would assume means that the mixin methods will not always be the same when ::asset() from Kirby\Filesystem\IsFile is called.

It looks like Kirby\Image\Image is a child of Kirby\Filesystem\File so it is probably safe to add @mixin File for Kirby\Filesystem\Asset. However, I don't know what would be needed to account for methods of Kirby\Image\Image. I'm not sure if @mixin annotations alone can do that, or if larger changes would be needed for static analysis tools to be able to understand what types of objects are used inside Kirby\Filesystem\Asset.

distantnative commented 1 month ago

@gaufde Does your IDE pick it up when you add the @mixin annotation to he Kirby\Filesystem\IsFile trait? That would probably be the best place but not sure if it works with traits.

The whole setup is a bit of a mess but also not easy to refactor, so we probably cannot make it ideal. E.g. I have no idea how to tell the IDE when File and when Image is used. Does it accept multiple mixins? Guess we have either the choice that the IDE doesn't know about certain methods on images or that the IDE thinks some methods are available on all files that are only available for images.

gaufde commented 1 month ago

@gaufde Does your IDE pick it up when you add the @mixin annotation to he Kirby\Filesystem\IsFile trait? That would probably be the best place but not sure if it works with traits.

Yeah, my IDE seems to pick it up equally well regardless of whether I add @mixin File to the Kirby\Filesystem\IsFile trait or the Kirby\Filesystem\Asset class.

Does it accept multiple mixins? Guess we have either the choice that the IDE doesn't know about certain methods on images or that the IDE thinks some methods are available on all files that are only available for images.

Yeah, using two mixins for File and Image seems to work fine for me on Kirby\Filesystem\IsFile. Though, it does of course introduce the conundrum mentioned above.

The whole setup is a bit of a mess but also not easy to refactor, so we probably cannot make it ideal. E.g. I have no idea how to tell the IDE when File and when Image is used.

I'm just learning about how some of this stuff works, but my guess is that the IDE won't be able to intuit what type of file will be returned based on the string passed to either the asset() helper function or Kirby\Filesystem\Asset directly.

What I think would be intuitive to me would be something like if asset() returned either a Kirby\Image\Image or a Kirby\Filesystem\File object directly. Then I could wrap my code in an if statement and use instanceof to narrow down the type of the object returned. I'm guessing it wouldn't work to do that directly, but hopefully that gives a sense of what would have made sense to me the first time I used this function.

It looks to me like the purpose of Kirby\Filesystem\Asset is mostly to add some path and URL methods to Kirby\Filesystem\File and Kirby\Image\Image objects as well as the FileModifications methods. @distantnative Do you think there would be a way to keep this functionality but make it more explicit that you will be getting either something that includes a File or a Image object so that developers can narrow down the types themselves using instanceof? Maybe there could be more than one type of Asset object?

getkirby / kirby