Skip to content
This repository has been archived by the owner on Aug 22, 2023. It is now read-only.

[RFC] ContextResolveEvent to allow context customization #84

Closed
wants to merge 2 commits into from

Conversation

ajgarlag
Copy link
Contributor

@ajgarlag ajgarlag commented Dec 9, 2020

With this feature, a user can write a listener to allow context customization when checking for a feature.

The dependency on the event dispatcher instance is optional, so it is backward compatible. Maybe it could be required in the next major version.

WDYT?

@coveralls
Copy link

Pull Request Test Coverage Report for Build 223

  • 26 of 28 (92.86%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.4%) to 98.812%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/Listener/AnnotationSubscriber.php 9 11 81.82%
Totals Coverage Status
Change from base Build 222: -0.4%
Covered Lines: 499
Relevant Lines: 505

💛 - Coveralls

2 similar comments
@coveralls
Copy link

Pull Request Test Coverage Report for Build 223

  • 26 of 28 (92.86%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.4%) to 98.812%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/Listener/AnnotationSubscriber.php 9 11 81.82%
Totals Coverage Status
Change from base Build 222: -0.4%
Covered Lines: 499
Relevant Lines: 505

💛 - Coveralls

@coveralls
Copy link

coveralls commented Dec 9, 2020

Pull Request Test Coverage Report for Build 223

  • 26 of 28 (92.86%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.4%) to 98.812%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/Listener/AnnotationSubscriber.php 9 11 81.82%
Totals Coverage Status
Change from base Build 222: -0.4%
Covered Lines: 499
Relevant Lines: 505

💛 - Coveralls

@ajgarlag
Copy link
Contributor Author

ajgarlag commented Dec 9, 2020

Another option to allow context customization without the event stuff is always passing a Context instance to the isActive check and leverage the ContextDecoratorInterface objects to decorate the context.

The main problem with this option is that the context decorator has no access to the feature name.

@ajgarlag ajgarlag marked this pull request as ready for review December 9, 2020 09:25
@migo315
Copy link
Contributor

migo315 commented Dec 9, 2020

I think this could be solved by a simple $context->replace('_feature', $name) here?
https://github.com/bestit/flagception-sdk/blob/master/src/Manager/FeatureManager.php#L51

Then you can create and tag your context decorator which has the feature name in $context->get('_feature').?
https://github.com/bestit/flagception-bundle/blob/master/src/DependencyInjection/CompilerPass/ContextDecoratorPass.php#L25

Then the feature name would always be present in the context and could be accessed everywhere.
Although the interface allows "Context" to be null, the FeatureManager always provides a context object. So as long as you use the services of the Flagception SDK, this should fit.

@ajgarlag
Copy link
Contributor Author

I think this could be solved by a simple $context->replace('_feature', $name) here?
https://github.com/bestit/flagception-sdk/blob/master/src/Manager/FeatureManager.php#L51

I like your proposition, PR open: bestit/flagception-sdk#24

@migo315
Copy link
Contributor

migo315 commented Dec 14, 2020

I've merged and released it. So we can close this PR?

@ajgarlag
Copy link
Contributor Author

You're right. Not needed anymore. Thanks.

@ajgarlag ajgarlag closed this Dec 14, 2020
@ajgarlag ajgarlag deleted the feature/context-event branch December 14, 2020 09:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants