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

NEW Allow use of new AnnotationTransformer for configuration #9006

Conversation

ScopeyNZ
Copy link
Contributor

Requires silverstripe/silverstripe-config#35

If that PR is merged then this PR enables configuration in SilverStripe with annotations. See that PR for more details.

@robbieaverill
Copy link
Contributor

@silverstripe/core-team this change has the potential the have quite an impact on the way dependency injection works in SilverStripe - worth reviewing when you have time

@ScopeyNZ
Copy link
Contributor Author

Would be good if the tests were left passing @ScopeyNZ

@maxime-rainville
Copy link
Contributor

Could we see a sample use case please?

@ScopeyNZ
Copy link
Contributor Author

#8564

@kinglozzer
Copy link
Member

I like it. I assume this PR complements #8564 rather than replaces it? I’d like to see some docs on @Inject(Foo,tag) - I think I’ve picked up what it’s used for from the test, but it’d be good to confirm 😄

Is upper case for the Injectable and Inject a convention? We currently use lower case for @config (which I think is an (unused) custom annotation of ours), should we be consistent one way or the other?

@emteknetnz
Copy link
Member

This pull request hasn't had any activity for a while. Are you going to be doing further work on it, or would you prefer to close it now?

@robbieaverill
Copy link
Contributor

It's really waiting on the core team's input. Let's chase that up cc @silverstripe/core-team

@ScopeyNZ
Copy link
Contributor Author

ScopeyNZ commented Sep 1, 2020

Yeah if this gets some positive feedback, I'll get the tests green. Right now I'm not sure the concept has the support yet.

@ScopeyNZ
Copy link
Contributor Author

ScopeyNZ commented Sep 1, 2020

FYI this is a requirement for #8564 which has a more specific use.

@GuySartorelli
Copy link
Member

It's looking like this is unlikely to get the support it needs, short of scheduling a live discussion or a vote or something of that nature. In the interest of not having things like this just sit around forever can we either call it dead or define some criteria under which it can either be considered accepted or rejected?

@GuySartorelli
Copy link
Member

As this hasn't had any comments since my last one I'm going to close this.

@GuySartorelli GuySartorelli deleted the pulls/4.5/scaffold-annotation-configs branch May 10, 2024 03:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants