There used to be 1 facet manager for every facet source.
Now we changed the this, and there is only one manager for all sources. This manager should be able to handle more than one source in one request (although this might be a rare situation).
The implementation right now feels buggy: what happens when two sources are active at the same time?
What needs to be done is the following:
- remove the protect variable facetsource_id from the manager.
- add a protected variable (array of facet sources) to hold all the facetsources that are used during the request.
- Refactor manager::alterQuery to alterQuery(&$query, source_id) so the query object is only fed to facets using the source_id facetsource.
- Refactor manager::updateResults(): each facetsource used in the request should be able to add the results to it's facet.
Comment | File | Size | Author |
---|---|---|---|
#9 | 2606562-9-separate-facet-processing-by-source.patch | 13.53 KB | ChristianAdamski |
#4 | 2606562-4-separate-facet-processing-by-source.patch | 12.85 KB | ChristianAdamski |
Comments
Comment #2
ChristianAdamski CreditAttribution: ChristianAdamski as a volunteer commentedI just ran into this. "My" core views integration provides two sources, one for exposed filters and one for contextual filters (thats GET parameters and URL parameters). There is a good chance one would want to use both mixed on the same page. And it fails, because the first source gets handed all facets to process, even the ones not belonging to it.
Comment #3
ChristianAdamski CreditAttribution: ChristianAdamski as a volunteer commentedLet's see what testbot has to add.
Comment #4
ChristianAdamski CreditAttribution: ChristianAdamski as a volunteer commentedRemoved the entire set/getFacetSourceId stuff. The manager is able to handle this by the facets it knows about.
Comment #5
borisson_A couple of smaller issues.
This changes makes sense and looks great.
https://www.drupal.org/coding-standards/docs
That means the first line of the docblock is now too long, I also think we should use
@var bool[]
to indicate that this is an array of booleans.The @return here isn't formatted as it should be:
Didn't we remove this method?
@param string $facetsource_id
This needs to be the full namespaced version.
Comment #6
ChristianAdamski CreditAttribution: ChristianAdamski as a volunteer commentedBefore I start to handle this:
1.) I use this alot! Is this realy removed? Not in current git at least.
2.)
I removed a lot of those. Do I get this right, that in the @param parts, we always want the full namespace? Or just in that one location?
Comment #7
borisson_re: #6:
https://www.drupal.org/coding-standards/docs
Comment #8
ChristianAdamski CreditAttribution: ChristianAdamski as a volunteer commented6.1. > Ah, ok. *phew* I removed that get/set FacetSource stuff from the Defaultmanager. The facets themself still need those alot.
6.2. > Oh boy. I was a bit trigger happy there because PHPStorm asked so nicely. Restoring them...
Comment #9
ChristianAdamski CreditAttribution: ChristianAdamski as a volunteer commentedOk, should fix all your remarks.
Comment #10
borisson_This looks great.
Comment #11
borisson_Awesome, thanks!