Skip to content

Possibility to define custom scrubber#554

Merged
danielmorell merged 1 commit into
rollbar:masterfrom
zinkovskiy:master
Nov 17, 2023
Merged

Possibility to define custom scrubber#554
danielmorell merged 1 commit into
rollbar:masterfrom
zinkovskiy:master

Conversation

@zinkovskiy
Copy link
Copy Markdown
Contributor

@zinkovskiy zinkovskiy commented Dec 27, 2021

Description of the change

In current implementation scrubbing fields is not flexible.
Sometimes we need to scrub particular cookie value without erasing all cookies, like suggested here: #477
So, why can't we add an option to define custom scrubber, especially when all the code has been written before? 🙂

Type of change

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

Related issues

@danielmorell danielmorell self-assigned this Dec 20, 2022
@danielmorell danielmorell self-requested a review December 20, 2022 20:09
@danielmorell
Copy link
Copy Markdown
Collaborator

Hi @zinkovskiy, sorry for the long wait on getting some feedback on this!

Rollbar already supports the ability to create a custom scrubber. The scrubber configuration name is already used to define a custom scrubber class. The class must implement the Rollbar\ScrubberInterface interface. I would recommend extending the Rollbar\Scrubber class as the easiest way to get up and going quickly.

Adding 'scrubber' to the list of options does not enable it, nor change the behavior of the SDK. Because of that I am going to close this PR.

@danielmorell danielmorell removed their request for review December 20, 2022 20:33
@zinkovskiy
Copy link
Copy Markdown
Contributor Author

Hi @zinkovskiy, sorry for the long wait on getting some feedback on this!

Rollbar already supports the ability to create a custom scrubber. The scrubber configuration name is already used to define a custom scrubber class. The class must implement the Rollbar\ScrubberInterface interface. I would recommend extending the Rollbar\Scrubber class as the easiest way to get up and going quickly.

Adding 'scrubber' to the list of options does not enable it, nor change the behavior of the SDK. Because of that I am going to close this PR.

Hi @danielmorell
I insist that scrubber should be in the list of options
Symfony bundle is used to integrate rollbar with symfony apps.
The bundle defines convenient way to configure rollbar.
The bundle obtain available configuration options using \Rollbar\Config::listOptions method, that just returns static array $options field.

scrubber is missing in this array, due to this the bundle does not allow to set scrubber option when configure rollbar
Error Unrecognized option "scrubber" under "rollbar". Available options are <...> will be shown when cache:clear symfony command is called

So, could you please get back to this issue and take a look on it one more time?

P. s.
I can create repository with example of symfony application with rollbar bundle if you need example of the bug.

@brianr brianr reopened this Nov 9, 2023
@danielmorell danielmorell self-requested a review November 17, 2023 17:48
Copy link
Copy Markdown
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.

Hi @zinkovskiy, I appreciate your persistence on this, and apologize for overlooking the use case you have identified. Thank you for not just giving up! Based on what you have said, I now think this is an important change and happily approve it.

@danielmorell danielmorell merged commit a69b14a into rollbar:master Nov 17, 2023
@zinkovskiy
Copy link
Copy Markdown
Contributor Author

Hi @zinkovskiy, I appreciate your persistence on this, and apologize for overlooking the use case you have identified. Thank you for not just giving up! Based on what you have said, I now think this is an important change and happily approve it.

Hi @danielmorell, thank you 🙏 Do you have estimation when these changes will be released?

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.

Scrubbing HTTP_COOKIE

3 participants