Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
core(entity-classification): integrate public-suffix-list into LH #15641
core(entity-classification): integrate public-suffix-list into LH #15641
Changes from all commits
182f299
2c03bd5
a4ba369
a26f8bc
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check warning on line 136 in core/lib/url-utils.js
Codecov / codecov/patch
core/lib/url-utils.js#L135-L136
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.
the subtlety here kinda goes over my head..
so we use the new PSL-powered getrootdomain in core/c/entity-classification
but out in report we use this method which leverages the boring-basic getrootdomain..
ya?
this does fix #15623 ? my brain is losing track of when the url data is set vs tweaked for display...
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.
On the audit side, we use the PSL library. We don't want to carry that much JS payload to the report though. Fortunately, there are only a few places on the report side that uses
getRootDomain()
— to convert a URL to its entity name for entity-grouping/lookup, and for legacy report third-party filtering, for example. For those, we already have keyed the entity-classification set with root-domains as entity names, so I rewrote them as entity-name comparisons.This weird
|string
return value is to support pre-10.0 LHRs, where we dont have entity classification data. For those, we fall back togetLegacyRootDomain
and convert them to string comparisons.It does fix #15623. The entity identified is the proper domain (vs.
mb.ca
before). Did you give it a try?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.
ok cool thanks.
nope but i knew you would've.