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

fix: respect ancestor property tags in model extensions #1037

Merged
merged 2 commits into from
Nov 19, 2021
Merged

fix: respect ancestor property tags in model extensions #1037

merged 2 commits into from
Nov 19, 2021

Conversation

spawnia
Copy link
Contributor

@spawnia spawnia commented Nov 18, 2021

  • Added or updated tests
  • Documented user facing changes
  • Updated CHANGELOG.md

Changes

Ensure the model extensions considers PHPDoc @property tags from ancestors, not just the model class itself.

Breaking changes

None.

@spawnia spawnia changed the title Respect ancestor property tags in model extension Respect ancestor property tags in model extensions Nov 18, 2021
@szepeviktor
Copy link
Collaborator

Hello Benedikt!
Thank you for your work.
Are you sure you want to encourage users to manually add many-many properties?
Larastan has a lot of effort recognizing magic properties in Laravel.

@spawnia
Copy link
Contributor Author

spawnia commented Nov 18, 2021

I think it is great that Larastan can recognize magic properties, but there are still situations that require an escape hatch:

  • BelongsTo or HasOne relations that are guaranteed to be non-null (backed by a non-nullable foreign key constraint)
  • specializations of generic implementations in a parent class, e.g. parent defines getIdAttribute(): int | string, but the ID is known to always be string in a given subclass

Larastan seems to recognize the necessity of this, given there is already an early return if a property annotation exists. I don't see why this logic should not extend to annotations from ancestors.

Copy link
Collaborator

@canvural canvural left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I had the same thing in mind actually.

Just left one remark. And after that I will refactor the test cases for this.

src/Reflection/ReflectionHelper.php Outdated Show resolved Hide resolved
@szepeviktor
Copy link
Collaborator

  • BelongsTo or HasOne relations that are guaranteed to be non-null (backed by a non-nullable foreign key constraint)

  • specializations of generic implementations in a parent class, e.g. parent defines getIdAttribute(): int | string, but the ID is known to always be string in a given subclass

I would still welcome these features in Larastan.

@canvural canvural changed the title Respect ancestor property tags in model extensions fix: respect ancestor property tags in model extensions Nov 19, 2021
@canvural canvural merged commit 50ef079 into larastan:master Nov 19, 2021
@canvural
Copy link
Collaborator

Thank you!

@spawnia spawnia deleted the respect-parent-property-tags branch November 19, 2021 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants