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 multiple class-string classes #157

Closed
wants to merge 1 commit into from

Conversation

voku
Copy link
Contributor

@voku voku commented Feb 5, 2022

e.g.: class-string<\Foo\Bar|\Foo\Lall>

⇒ currently this will throw an exception like \Foo\Bar|\Foo\Lall is not a class string

@orklah
Copy link
Contributor

orklah commented Feb 5, 2022

Note that Psalm doesn't like that either: https://psalm.dev/r/06ca7972ec

PHPStan is fine with it though

e.g.: `class-string<\Foo\Bar|\Foo\Lall>`
@voku voku force-pushed the fix_class_string branch from 8f4e85c to 3d6ce9f Compare February 6, 2022 15:00
@jaapio
Copy link
Member

jaapio commented Mar 15, 2022

Thanks a lot for this PR.

I do get what you are trying to achieve here. But I do not like the extra complexity it takes to get this all done. The maintainability of this parser is becoming an issue more and more. And I'm wondering how to change that because I understand that the type systems supported by psalm and phpstan are getting more complex as well.

maybe we should consider todo a rewrite of the parser to get it where I want it to be. I think the last thing any of us is waiting for are bugs in this lib, with high consequences for all consumers of this library.

This doesn't mean I will never merge this pr, but I want to check first if this can be solved in a more convenient way.

@jaapio
Copy link
Member

jaapio commented Mar 17, 2023

Since you created this PR our project has been changed, we have done some major rebuilding of the internals of this library. I think we should put this on a roadmap to support this notation. But that requires some breaking changes I think... as it means that the class-string and interface-string should possibly contain a type.

I don't like the solution with an aggregate type of *-string objects. as it does not reflect the original type close enough to cover all situations. And I'm not sure how this will impact the behavior of the consuming projects.

I feel sorry to say this, but at this moment I think we cannot support this in the correct way.

@jaapio
Copy link
Member

jaapio commented Mar 17, 2023

#186

@jaapio jaapio closed this Mar 17, 2023
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.

3 participants