Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Based on the discussion at http://drupal.org/node/1388666#comment-5414652, we can discuss ways to simplify the FacetapiDependencyFacet::getEnabledFacets() in this thread.
Comment | File | Size | Author |
---|---|---|---|
#1 | facetapi_bonus-1389510-1.patch | 1.43 KB | cpliakas |
Comments
Comment #1
cpliakas CreditAttribution: cpliakas commentedOverall the FacetapiDependencyFacet::getEnabledFacets() uses the correct API functions to gather the enabled facets. There are a few very minor items that could be cleaned up, and I would recommend getting the searcher via the adapter as opposed to the form (although it is a perfectly sane approach). See the attached patch for the idea.
Comment #2
cpliakas CreditAttribution: cpliakas commentedAnother item I forgot to mention regarding the patch above... the dependency already gives you the facet in the
$this->facet
property, so we can use that instead of getting the name from the form. Again a minor item because the form approach is sane, just adds some consistency and code clarity.Comment #3
danielnolde CreditAttribution: danielnolde commentedAh, niccce! That's exactly what i had in mind - using all the indirect data from the form was a clumsy workaround for not knowing the proper way to access or get the data directly - thanks, Chris!
reviewed, tested and committed (ahh - you have commit rights, too - do you want to do commits of reviewed patches directly for future issues?)
Comment #4
cpliakas CreditAttribution: cpliakas commentedThanks for committing.
That's OK. I want you to drive this module as much as possible. Now that your application got accepted, I will definitely be less active in the queue and look forward to see where you take this module. Of course I am always available for advice on the inner workings of Facet API if you want to get a second opinion on something.
Thanks,
Chris