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

Possibility to use service as person_fn rather than a static function #93

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

zinkovskiy
Copy link

@zinkovskiy zinkovskiy commented Dec 7, 2023

Description of the change

This PR allow to use service as person_fn

I created PersonProvider service that is used by default

To use custom person provider you need to define your own service with alias rollbar.person-provider in services.yml of your application, for example:

  rollbar.person-provider:
    class: App\Service\Rollbar\Provider\PersonProvider

No need to define person_fn option (thx to DI), the service will be used automatically, but if person_fn is defined, then it will be used

Logic of obtaining user info is not changed.

I've also discovered, that security.token_storage and serializer are private and cannot be obtained from container, so I passed it to Rollbar\Symfony\RollbarBundle\Factories\RollbarHandlerFactory constructor

Type of change

  • Bug fix (non-breaking change that fixes an issue)

Related issues

Checklists

Development

  • All tests related to the changed code pass in development

Code review

I tested these changes on symfony 5.4 and symfony 6.4, it works well

@danielmorell could you please take a look on it?

@zdavis-rollbar
Copy link

@zinkovskiy, we are looking for a maintainer or owner of the repo. Would you like to participate?

@zinkovskiy
Copy link
Author

@zinkovskiy, we are looking for a maintainer or owner of the repo. Would you like to participate?

Hi @zdavis-rollbar yes, I'm ready to maintain. contact me anytime when you have time, we are interested in keeping the project up to date

@waltjones
Copy link

@zinkovskiy Thank you for the PR. Please add tests for the new functionality, and run tests locally to ensure CI will pass.

@zinkovskiy
Copy link
Author

@zinkovskiy Thank you for the PR. Please add tests for the new functionality, and run tests locally to ensure CI will pass.

Hi @waltjones, yes, I missed it. Now it's added
Hope it helps to accept changes 🙂

Copy link
Collaborator

@danielmorell danielmorell left a comment

Choose a reason for hiding this comment

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

Hey Anton, thank you for your work on this and your patience in getting this reviewed!

I appreicate how customizable and extensible you have made this. Great work!

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.

Possibility to use service as person_fn rather than a static function
4 participants