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

ENH Correctly parse SomeClass::class syntax in textcollection #10577

Conversation

GuySartorelli
Copy link
Member

This is a pretty naive approach - there was some discussion in the parent issue about refactoring all this to use an existing php parser, but this was quicker and got the job done for now.

It makes the same assumptions mentioned in #7647 (comment):

  • SomeClass has been imported via a use statement, or;
  • SomeClass exists in the same namespace (global or otherwise) as the current class

Parent issue

@GuySartorelli GuySartorelli force-pushed the pulls/4/textcollector-class-notation branch 2 times, most recently from 797f817 to 03c7d8c Compare November 10, 2022 22:14
@GuySartorelli GuySartorelli force-pushed the pulls/4/textcollector-class-notation branch from 03c7d8c to 521c817 Compare November 10, 2022 22:38
@michalkleiner
Copy link
Contributor

Do we want to add an anonymous class to the test as well since we recently had an issue with anonymous class instantiation? It might not be directly relevant but possibly worth having in the tests.

@GuySartorelli
Copy link
Member Author

GuySartorelli commented Nov 10, 2022

I think this is a very different scenario than the injector issue we had - while it does make sense in some situations to use injector to replace some concrete class or service with an anonymous class, I can't see any scenario where someone would want to use an anonymous class as a key for translations. Honestly I think it's totally fine to just leave that behaviour undefined. It's not something we would want to support, I don't think.

I will add tests for it if you insist or someone else agrees with you, though I'd need some guidance as to what the expected behaviour should be in order to test it.

@michalkleiner
Copy link
Contributor

Nah, all good, was just a question if it made sense.

Copy link
Contributor

@michalkleiner michalkleiner left a comment

Choose a reason for hiding this comment

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

Do we want this as a fix in 4.12 as it fixes a bug?

@GuySartorelli
Copy link
Member Author

Does it fix a bug or does it just give the text collection parser an ability it didn't have before?
Either way I'm not terribly fussed as to whether it gets into 4.12 or not.... not sure how others feel.

@michalkleiner
Copy link
Contributor

Let's leave the target branch as is.

@michalkleiner michalkleiner merged commit da06a2d into silverstripe:4 Nov 24, 2022
@michalkleiner michalkleiner deleted the pulls/4/textcollector-class-notation branch November 24, 2022 21:28
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.

2 participants