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

IDE support for Filesystem\Asset proxied methods #6598

Open
gaufde opened this issue Aug 2, 2024 · 2 comments
Open

IDE support for Filesystem\Asset proxied methods #6598

gaufde opened this issue Aug 2, 2024 · 2 comments
Labels
type: enhancement ✨ Suggests an enhancement; improves Kirby

Comments

@gaufde
Copy link

gaufde commented Aug 2, 2024

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

  1. Use an IDE with good code "understanding" like PhpStorm (or probably VS Code with Intelephense) that is configured to inspect for missing methods.
  2. Paste asset('logo.svg')->read() into a template
  3. 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: lukaskleinschmidt/kirby-types#4 (comment)

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 distantnative changed the title False positives for missing mixin methods on Kirby\Filesystem\Asset IDE support for Filesystem\Asset proxied methods Aug 6, 2024
@distantnative distantnative added the type: enhancement ✨ Suggests an enhancement; improves Kirby label Aug 6, 2024
@distantnative
Copy link
Member

@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
Copy link
Author

gaufde commented Aug 11, 2024

@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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement ✨ Suggests an enhancement; improves Kirby
Projects
None yet
Development

No branches or pull requests

2 participants