-
Notifications
You must be signed in to change notification settings - Fork 13
Automatically make result_attributes indexable #1163
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
Conversation
| ...$indexableAttributeCodes, | ||
| ...$this->superAttributeCodes, | ||
| ]); | ||
| ...config('rapidez.searchkit.result_attributes'), |
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.
If we were to skip wildcards this would be the only addition.
Adding wildcards makes it feature-complete though
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.
Do we want to support wildcards? I can imagine maybe you only want specific attributes indexed, but you can then use the wildcard in your result attributes to get all of the ones you've indexed?
Imagine I'm a lazy developer and I just set the result_attributes to * so I can get all of my indexed data. I probably wouldn't expect that change to also then cause literally everything to get indexed.
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.
We're already using it, see:
core/config/rapidez/searchkit.php
Line 35 in 5224f79
| 'super_*', |
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.
My question is not whether it works (I know it works), my question is: do we actually want to actually directly cause a wildcard in the result_attributes to result in indexing (and thus exposing) a bunch of values we might not have wanted. To me this seems like an easy way to accidentally get unexpected data leakage.
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.
Check, yes we want wildcards. As already used for all super attributes which should always be exposed. Creating a new one just works after a reindex; no need for a developer.
|
Please check the tests 😇 |
Previously adding an attribute to
result_attributeswouldn't mean it would actually get indexed, only that if it is indexed it can be used.This change makes it so
result_attributeswill always be added to indexable values.