Skip to content

Create a IsA expression #396

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

hgraca
Copy link
Contributor

@hgraca hgraca commented Jul 26, 2023

This will allow for testing if a class extends or
implements another code unit, anywhere in the inheritance tree.

Comment on lines 47 to 48
\is_a($theClass->getFQCN(), $allowedFqcn, true)
|| \is_subclass_of($theClass->getFQCN(), $allowedFqcn)

Choose a reason for hiding this comment

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

Do we really need to check both? Both do the same, except that is_a() also returns true for the class itself (like is_a(A::class, A::class)), AFAIK, so is_subclass_of() should never be called.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

unfortunately, is_a doesnt check for interface implementation, and is_subclass_of doesnt check for same class. :(

Choose a reason for hiding this comment

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

unfortunately, is_a doesn't check for interface implementation

I think it does: https://3v4l.org/91Yt8

AFAIK, is_a does the same as instanceof (and it is even more powerful, since it can check string class names and not only instances, if the third parameter is true).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

HAH! u r correct, I will fix this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed and force pushed

@hgraca hgraca force-pushed the feat/isa_expression branch from 7b00915 to 5143cff Compare July 27, 2023 08:50
@hgraca hgraca requested a review from jdreesen July 27, 2023 08:51
This will allow for testing if a class extends or
implements another code unit, anywhere in the inheritance tree.
@hgraca hgraca force-pushed the feat/isa_expression branch from 5143cff to cafad1a Compare August 5, 2023 20:45
@micheleorselli micheleorselli self-assigned this Aug 22, 2023
@raffaelecarelle
Copy link
Contributor

Hello, this PR is useful. When can it merge?

@micheleorselli
Copy link
Member

Hello, this PR is useful. When can it merge?

When we used to support older php versions we wanted to avoid relying on checks triggering the autoload and parse the code statically. Not sure it still makes sense, so potentially this is a good addition. At this point I am wondering if we should just update Extend/NotExtend Implement/NotImplement. wdyt @AlessandroMinoccheri @fain182?

@hgraca
Copy link
Contributor Author

hgraca commented Apr 13, 2025

@micheleorselli

avoid relying on checks triggering the autoload

That is why I opened this other PR but never got it merged.
This would effectively put the decision on the user; if they wanted to use an expression that would need to trigger the autoload, they would be able to, which is not the case at the moment.

That PR just adds an IF statement so we don't try to define PHPARKITECT_COMPOSER_INSTALL more than once, which triggers PHP errors.
I don't see a negative point in merging it, but if it is not desirable, let me know so I can close that PR.

@AlessandroMinoccheri
Copy link
Member

For me, the PR is adding value.
What is your idea of extending Extend/NotExtend and Implement/NotImplement @micheleorselli ?

Maybe we can consider both ways to understand what is the correct one.

@micheleorselli
Copy link
Member

micheleorselli commented Apr 15, 2025

As of now, there is a known limitation when we parse files building ClassDescription objects as we do not walk the inheritance tree, so the analisys stops at the class we currently parsing. This means that, given a ClassDescription object, the extends and implement attributes contains only classes that are directly extended or implemented.

If I have a rule saying "class X should not extend class Y" is that the expected behaviour? I doubt that 🤔

One possibile approach would be having two rules like

  • Extends, which work using is_a and triggering autoload
  • ExtendsDirectly which behaves like the current Extends

this also would work for implements for interfaces, use for traits, and so on

wdty? @AlessandroMinoccheri @fain182

@fain182
Copy link
Collaborator

fain182 commented Apr 15, 2025

When we used to support older php versions we wanted to avoid relying on checks triggering the autoload and parse the code statically.

I think we need to ensure that autoloading works with all our installation methods (Composer, PHAR, Docker); otherwise, it's a good idea. (I'm even open to deprecating some of the installation methods.)

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.

6 participants