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 for issue #81 Traits Composed from Traits #102

Closed
wants to merge 1 commit into from

Conversation

hboomsma
Copy link
Contributor

Fix for issue #81

Use statements are now also imported for properties
and methods from traits in traits. Conflict resolution
and aliases are respected.

For backwards compatibility reasons the use statements
from the declaring class are still merged. PHP itself 
does not show this behaviour regarding to imported types
in traits.

For the methods in traits I use \ReflectionMethod::getFileName()
and then parse the file since this is the only way I can think of to
get the right declaration location in the face of imported traits using
conflict resolution. This same method is also possible for the
properties if you would prefer it.

Use statements are now also imported for properties
and methods from traits in traits. Conflict resolution
and aliases are respected.

For backwards compatibility reasons the use statements
from the declaring class are still merged. PHP itself 
does not show this behaviour regarding to imported types
in traits.
@mikeSimonson mikeSimonson added this to the v1.3.2 milestone Jan 1, 2017
@mikeSimonson mikeSimonson modified the milestones: v1.4.1, v1.3.2 Feb 24, 2017
@Ocramius Ocramius removed this from the v1.4.1 milestone Jul 22, 2017
@tommygnr
Copy link

It would be great to get this merged if possible. It's a very annoying bug.

@tommygnr
Copy link

Bump @Ocramius @guilhermeblanco

@johnpancoast
Copy link

It's been just shy of 2 years since this was submitted. Is there anything we can do to help get this merged?

@Ocramius
Copy link
Member

Ocramius commented Aug 2, 2018

I simply completely missed this (among the 186 open PRs I'm mentioned in).

Keeping a tab open, can't promise much.

@johnpancoast
Copy link

johnpancoast commented Aug 2, 2018

@Ocramius, thanks for the quick reply. I can totally understand if this task got forgotten due to other tasks taking priority. This bug is probably a lower priority since there's a workaround to add use stmts in the trait that depends on the others. It's just that it's not the prettiest workaround. As long as we add notes for why the uses are necessary, it's not that bad I suppose though.

Apologies if I seemed too blunt, but I'm glad this is at least back on your radar and I understand if it's still lesser priority than others. Let me know if I can help. Thanks! :)

@wadjeroudi
Copy link

@Ocramius I think this PR should have a higher priority now, bcz the use of tools like php cs fixer has become democratized. Thx.

@Majkl578
Copy link
Contributor

This is what PHPStan is using, we should use something similar or based on that: https://github.com/phpstan/phpstan/blob/0.10.6/src/Reflection/Php/PhpClassReflectionExtension.php#L365-L437

@Majkl578
Copy link
Contributor

Ok, this gets REALLY tricky when only using PHP's Reflection:

  • It's not possible to reliably detect whether a method actually comes from a trait or was shadowed by another method with same signature in the class. While this would work in most cases, it wouldn't in an edge case when all code is on a single line in the same file: https://3v4l.org/h9Uq1
  • There is no way to know where property comes from.

I don't think support for traits is deterministically doable without introspecting the code itself using Nikic's PHP Parser (which is something I would like to avoid, but am open to do since it would also be useful for use parser). /cc @Ocramius?

@Ocramius
Copy link
Member

If we can keep traits out of the game completely, I'm a happy coder anyway.

I realise many folks rely on them, but they are really really messy overall. If we can harden the tests well enough to support all edge cases, then fine, but otherwise we shouldn't ship something half-baked.

@Ocramius
Copy link
Member

The baseline is: loads and loads of test scenarios for the traits (for properties, methods and class-level annotations), for following scenarios (which are the ones I can think of immediately):

  • class using trait, annotation in class, no collision
  • class using trait, annotation in trait, no collision
  • class using trait, annotation in trait, collision
  • class using trait, annotation in class, aliased import
  • class using trait, annotation in trait, aliased import
  • class using trait using trait, annotation in second trait, no collision
  • class using trait using trait, annotation in second trait, collision
  • class using trait using trait, annotation in second trait, aliased import

@Majkl578
Copy link
Contributor

Problem is that we can't even reliably leave traits out of equation. Unless you bail out immediately when a class uses there may or may not be trait stuff incorportated (can be shadowed by class or not) and this is not properly detectable by Reflection. :S
Also like it or not, people use traits for weird stuff and I know a lot of scenarios where people use stuff like IdentifiedTrait for ORM entities to define auto-integer $id, or UuidIdentifiedTrait.

@Majkl578
Copy link
Contributor

Current behaviour is IMO even worse: it ignores uses from trait files and always only uses uses from the class importing trait: https://github.com/doctrine/annotations/blob/master/tests/Doctrine/Tests/Annotations/Fixtures/ControllerWithTrait.php#L5-L6

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

7 participants