An issue has been posted at #1152368: Implement API change to prevent the need for overrides and allowing Facet API to better control application flow to facilitate changing the application flow which at least three other issues depend on. The API change will facilitate less overrides in the adapter code allowing for Facet API to make flow changes without disturbing the backends' adapter implementations.

CommentFileSizeAuthor
#11 apachesolr-1152382-11.patch4.47 KBcpliakas
#3 apachesolr-1152382-3.patch4.45 KBpwolanin
#1 apachesolr-1152382-1.patch5.08 KBcpliakas

Comments

cpliakas’s picture

cpliakas commented
Status: Active » Needs review
StatusFileSize
new apachesolr-1152382-1.patch5.08 KB

The attached patch works with the patch posted at http://drupal.org/node/1152368#comment-4447848.

pwolanin’s picture

pwolanin commented

Taking a quick looks - why is 

function apachesolr_add_facet_filters()

external to the adapter class?

pwolanin’s picture

pwolanin commented
StatusFileSize
new apachesolr-1152382-3.patch4.45 KB

Actually - I take that back.

The code could just go to the QueryType plugin perhaps - since the Date facet doesn't actually support the OR operator.

cpliakas’s picture

cpliakas commented

That seems fine to me, but looking forward if someone wants to implement a range or wildcard query type plugin, would this code need to be reused? In that case, putting it in the adapter like you originally mentioned might be the best approach so it could be reused. There is no reason for it being a separate function other than it being 1AM when i wrote the code.

pwolanin’s picture

pwolanin commented

Well, the date one is adding e.g. '.facet.date.gap' so the range ones will be a little different.

I think this is good enough for now - we can always pull out common code if we find we have duplication.

cpliakas’s picture

cpliakas commented
Status: Needs review » Reviewed & tested by the community

Sounds good. Thanks for being open to the change.

pwolanin’s picture

pwolanin commented

Before committing this - do we need to do something in terms of API versioning, or are we jsut going to document a coupled release?

pwolanin’s picture

pwolanin commented

Before committing this - do we need to do something in terms of API versioning, or are we jsut going to document a coupled release?

cpliakas’s picture

cpliakas commented

I would say since they are in beta and only 18 people are reportedly using the beta6 version, a documented, coupled release would be fine. If this isn't acceptable, then I feel as if the most common use case would be for Apache Solr to be updated before Facet API. In that case we could add a condition in the apachesolr_do_query() function that could be phased out when a stable version is released. That is really the only area that would need version specific code.

In terms of a Facet API release, there are still some known issues but I think there are enough fixes to be able to push a new release whenever. This is also the last architectural change I plan on making, so we shouldn't have to coordinate releases this tightly again.

cpliakas’s picture

cpliakas commented
Status: Reviewed & tested by the community » Needs work

Just noticed an issue with patch #3. $facet is undefined and must be changed to $this->facet.

cpliakas’s picture

cpliakas commented
Status: Needs work » Needs review
StatusFileSize
new apachesolr-1152382-11.patch4.47 KB

Attached patch resolves the issue.

pwolanin’s picture

pwolanin commented
Status: Needs review » Reviewed & tested by the community

Indeed, sorry I missed that.

pwolanin’s picture

pwolanin commented
Status: Reviewed & tested by the community » Fixed

committed

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.