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

PHP8.2 - Interface property annotation not found inside class #8550

Closed
lcharette opened this issue Dec 18, 2022 · 24 comments
Closed

PHP8.2 - Interface property annotation not found inside class #8550

lcharette opened this issue Dec 18, 2022 · 24 comments

Comments

@lcharette
Copy link

lcharette commented Dec 18, 2022

Bug report

Similar to #8537, when a property is documented in an interface @property annotation, it's not recognized by classes using this interface. This is not an issue on PHP 8.0 and 8.1.

Code snippet that reproduces the problem

https://phpstan.org/r/75fa6a47-cbc4-4eb3-b5eb-cf4851f6b805

Access to an undefined property SomeInterface::$var.

Expected output

No errors!

Did PHPStan help you today? Did it make you happy in any way?

PHPStan helped me become a better PHP dev :)

@ondrejmirtes
Copy link
Member

I'm hesitant to support this, because @property above an interface doesn't mean anything - it doesn't mean that all interfaces should declare this property...

@lcharette
Copy link
Author

I understand. However this is a big shortcut when there's a lot of magic methods and properties involved, like Laravel's Model for example.

Another example with magic methods : https://phpstan.org/r/02a7420c-a73a-473d-b5f6-241e19e0e387

Using the property on the Some class is not an option, since Sample only knows about SomeInterface. For example (expected failure) : https://phpstan.org/r/4dc9be64-0aff-41ea-a9db-01a89e3a6f3a

This is simply a shortcoming of PHP Interfaces.

@LastDragon-ru
Copy link

like Laravel's Model for example

For laravel models you can add @property into model. For interfaces you can add getters (which is more error safe), OR magic methods -> @property seems will work in this case.

https://phpstan.org/r/f73ee926-ee7b-4464-96f2-3708e813ee8c

@lcharette
Copy link
Author

Adding @property to Model doesn't solve this, as my class uses the interface. Duplicating the getter/setter in the interface sound counterproductive. It force the model to use the getter/setter, while some property could be defined as normal properties.

But your exemple shows something interesting:

  1. With @property and with getter/setters : Work on all versions
  2. Without @property and with getter/setters : Doesn't work in any version
  3. With @property and without getter/setters : Doesn't work on PHP 8.2 only

It makes no sense that 3 doesn't work if 1 does while 2 doesn't.

@johanmolen
Copy link

Having the same problem since updating to PHP8.2. @lcharette did you find a fix? Besides the getter/setter?

@wpmvangroesen
Copy link

wpmvangroesen commented Feb 8, 2023

This was allowed with versions before PHP 8.2. Maybe shouldn't have because the language itself doesn't support it. Although you could argue the language should probably allow this, like many other languages have support for properties. The PHP manual states that you should use abstract classes instead. But that's not always what you want.
PHP actually does have support for constants in interfaces, so i would really like to see properties as well. If we could be able to use @property definitions for now, that would be great!

@josefsabl
Copy link

I'm hesitant to support this, because @property above an interface doesn't mean anything - it doesn't mean that all interfaces should declare this property...

We also commonly use this technique.

@eithed
Copy link

eithed commented Feb 17, 2023

Adding #[\AllowDynamicProperties] works for interfaces as well as shown here: https://phpstan.org/r/ecc5a472-e2a3-4c8a-a314-51124b72edee although I don't know how to feel about this given

it doesn't mean that all interfaces should declare this property

I guess it would be good if phpstan would report if indeed given property was defined on the interface but is not defined on the class, ie if https://phpstan.org/r/57b63210-3875-4f28-871c-cf97d91ef779 would error


edit: apologies - idk if I was running 8.2 or not, but today I'm getting Fatal error: Cannot apply #[AllowDynamicProperties] to interface; I don't know why that's not happening on share link

@lcharette
Copy link
Author

lcharette commented Apr 17, 2023

Considering my case is mostly Database Models, I could use the Universal object crates, add each model Interface in my config and the error would be suppressed. However, this isn't a perfect solution. If my Foo interface only declare @property int $id, and "Universal object crates" is used, $foo->bar won't throw an exception (I can't add a playground to demonstrate this because config can't be declared on Playground). It's true because of how Laravel define its setter, assigning a dynamic var to the implementing class won't throw an error on the PHP execution. However, PHPStan should still obey the interface declaration when using $foo->bar.

No other solution provided by the new blog post seams applicable in this case. As @eithed noted, AllowDynamicProperties is not an option, as PHP return this error when dealing with Interfaces (and PHPStan should probably too) :

PHP Fatal error:  Cannot apply #[AllowDynamicProperties] to interface

Consider this other example :

This is still an issue IMO. On the new blog post, in Add @property PHPDoc, a warning about this issue should be added too.

lcharette added a commit to userfrosting/sprinkle-account that referenced this issue Apr 17, 2023
lcharette added a commit to userfrosting/sprinkle-core that referenced this issue Apr 17, 2023
lcharette added a commit to userfrosting/sprinkle-admin that referenced this issue Apr 18, 2023
@solomonjames
Copy link

I am currently seeing this issue on 8.2, without it being an interface. Something as simple as this: https://phpstan.org/r/7a7162bc-f3fc-45c3-b3c6-dadf6969e38b

@ondrejmirtes
Copy link
Member

@solomonjames
Copy link

@ondrejmirtes Ok, I just realized my specific problem... The vendor library has their docblocks like so

/**
 * @property string name
 */

instead of

/**
 * @property string $name
 */

I was able to resolve it with a stub.

Thank you!

@herndlm
Copy link
Contributor

herndlm commented Apr 28, 2023

@solomonjames please open a PR in the vendor lib for that change too if you can and didn't do already 😊

@JasonEleventeen
Copy link

JasonEleventeen commented May 24, 2023

This is fixable by patching https://github.com/phpstan/phpstan-src/blob/19c838bbe9576c83c95ebb1f9fedd5e5b8664306/src/Reflection/ClassReflection.php#L380

    public function hasProperty(string $propertyName): bool
{
	if ($this->isEnum()) {
		return $this->hasNativeProperty($propertyName);
	}

	foreach ($this->propertiesClassReflectionExtensions as $i => $extension) {
		if ($i > 0 && (!$this->allowsDynamicPropertiesExtensions() && !$this->isInterface())) {
			continue;
		}
		if ($extension->hasProperty($this, $propertyName)) {
			return true;
		}
	}

	return false;
}

would a PR to allow this be accepted? Could put it behind a feature Flag if required

@JasonEleventeen
Copy link

JasonEleventeen commented May 25, 2023

We have used this to workaround the issue for now, all our concrete classes are laravel models so they have __set

  /**
   * Inherit from this interface if your interface has properties
   */
  interface BaseInterface
  {
      // HACK: workaround for phpstan no longer allowing interfaces to have properties, without magic methods
      // the concrete class will have to have __isset method
      /**
       * @param string $key
       * @return mixed
       */
      public function __isset($key);
  }

@brysem
Copy link

brysem commented Aug 24, 2023

The hack @JasonEleventeen applied should be unnecessary.

I am in a similar situation that interfaces are annotated with @property for Eloquent models in Laravel. Adding the properties are very useful when writing packages for models that can be replaced with your own implementation but need to abide to the interface.

Applying the @mixin \Eloquent to the interface should inherit the magic methods __set and _get from the Eloquent model for the interface.

The documentation indicates that the annotated properties are detected if these magic methods are present:
Have __get and/or __set magic methods for handling dynamic properties.

Therefore, I expect it to work now with the mixin applied.

However, it still does not work. Applying the hack from @JasonEleventeen does fix it though.
Despite the Eloquent model also implementing the magic method __isset.

Furthermore, it is not detecting any magic properties from the @mixin \Eloquent model anymore either.
An example is that it no longer understands what ->exists or ->wasRecentlyCreated is anymore when accessing these properties.

This seems more like a bug than a feature request. @ondrejmirtes

Example code.

/**
 * @property-read Collection<int,Statusable> $statuses
 *
 * @mixin \Eloquent */
interface StatusableInterface
{
    public function statuses(): HasMany;
}

Access to an undefined property App\Interfaces\StatusableInterface::$statuses

@ondrejmirtes
Copy link
Member

This is now possible to solve with @phpstan-require-extends and @phpstan-require-implements.

See an example: https://phpstan.org/r/2dec4d59-482d-4894-ba05-0c2453e28f2b

When the typehinted interface is implemented by a class, it requires the class to extend a class in @phpstan-require-extends.

The class can then declare the property as native or via other means like @property.

@JasonEleventeen
Copy link

@phpstan-require-extends does not fix the general underlying issue, we might as well use DTO's instead if we have to write a base model for every interface..

@ondrejmirtes
Copy link
Member

@JasonEleventeen The question you should ask is: How do the properties work in the actual app? When you typehint the interface and access those properties, what's the actual implementation that makes them work? That's your class that you should put in @phpstan-require-extends.

@JasonEleventeen
Copy link

@JasonEleventeen The question you should ask is: How do the properties work in the actual app? When you typehint the interface and access those properties, what's the actual implementation that makes them work? That's your class that you should put in @phpstan-require-extends.

https://www.digitalocean.com/community/conceptual-articles/s-o-l-i-d-the-first-five-principles-of-object-oriented-design#liskov-substitution-principle

If the interface depends on the model whats the point of the interface?

@ondrejmirtes
Copy link
Member

@JasonEleventeen Interfaces in PHP do not support properties at all. We found a way to do it but it requires usage of @phpstan-require-extends. I don't think this breaks or is relevant to LSP at all.

@JasonEleventeen
Copy link

PHP does not support generics either, that's part why we use phpstan and this did work previously, if its a definite will not fix that's fine, but I don't think the @phpstan-require-extends workaround is an actual fix

@ondrejmirtes
Copy link
Member

@JasonEleventeen I'd like to know about your specific use case #8550 (comment). You must have "some" reason to "declare" properties on an interface. I'd like to know how it works in your code. Feel free to open a new discussion about this.

Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests