-
Notifications
You must be signed in to change notification settings - Fork 121
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
Replace Whitelist with Safelist #494
Conversation
@@ -50,7 +50,7 @@ class Config | |||
'capture_email', | |||
'root', | |||
'scrub_fields', | |||
'scrub_whitelist', | |||
'scrub_safelist', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know the assignment here says $options
is private, but according to the docs the user has access to set this. Will this change affect existing users?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@waltjones, you bring up a good point. I am not totally sure how options are exposed. Let me look into it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two points:
-
We can add a new key, but we shouldn't remove
scrub_whitelist
at the same time: there should be at least a minor version increment between them. -
I prefer
allowlist
instead ofsafelist
, because the opposite is (IMO) clearer:denylist
vsunsafelist
. Is the "safelist" nomenclature a standard in Rollbar?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
allowlist
was considered, but the consensus was to standardize on safelist
(and blocklist
.)
The CH story for each of the SDKs is to only deprecate, not remove, public facing names until the next major version release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good except what @waltjones pointed.
We are exposing scrub_whitelist
and it will break backward compatibility, which should be fine if document it properly and give notice for depreciation.
Ref: https://docs.rollbar.com/docs/php-configuration-reference
I have marked this as "Status: 2 - Accepted" because the concept and need is understood; there are still some questions on the implementation, though: see above. |
phpunit.xml
Outdated
<directory suffix=".php">./src</directory> | ||
</whitelist> | ||
</safelist> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is not relevant to the implementation. Needs reversion.
1a7ea98
to
7b31e88
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
This is a BREAKING CHANGE. Consumers currently relying upon
scrub_whitelist
orsetWhitelist
will need to update their code or configuration toscrub_safelist
andsetSafelist
respectively.