We should copy the indexable bundles (those that were selected by the user) to the newly enabled acquia search environment so the transition from a self-hosted to Acquia Search index is as smooth as possible.

This issue is in line with #1783796: We should manage excluded, not indexed, bundles and #1790894: Cloning an environment doesn't clone the bundles to be indexed

CommentFileSizeAuthor
#22 1791338-21-fu.patch1.92 KBpwolanin
#21 1791338-20-fu.patch1.19 KBpwolanin
#20 1791338-19-fu.patch705 bytespwolanin
#14 1791338-13.patch4.39 KBpwolanin
#13 1791338-12.patch4.27 KBNick_vh
#12 1791338-12.patch4.27 KBpwolanin
#6 1791338-6.patch4.14 KBNick_vh
#3 1791338-3.patch3.84 KBNick_vh
#1 1791338-1.patch2.25 KBNick_vh
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Nick_vh’s picture

Nick_vh
he/him
Location Ghent
CreditAttribution: Nick_vh commented
Status: Active » Needs review
FileSize
1791338-1.patch2.25 KB
pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Status: Needs review » Needs work

re:

variable_del('apachesolr_default_environment');

We shouldn't do that unless 'solr' exists as an environment. If it doesn't we might need to re-create it.

Nick_vh’s picture

Nick_vh
he/him
Location Ghent
CreditAttribution: Nick_vh commented
FileSize
1791338-3.patch3.84 KB

Tested this patch - just don't like the fact that I have to replicate some code from the main module - but it's not a big deal.

Also, we are now depending on the latest dev version of apachesolr since i am referring to the clear cache function. Make sure you include that in the release notes

Nick_vh’s picture

Nick_vh
he/him
Location Ghent
CreditAttribution: Nick_vh commented
Status: Needs work » Needs review

let's get the testbot some work

Nick_vh’s picture

Nick_vh
he/him
Location Ghent
CreditAttribution: Nick_vh commented

second try. I'm depending on a clear cache function here, I should probably not do that

Nick_vh’s picture

Nick_vh
he/him
Location Ghent
CreditAttribution: Nick_vh commented
FileSize
1791338-6.patch4.14 KB

Decided I should not depend on a function that is not there in previous versions

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

Looks better- I agree we shouldn't depend on new functions.

pwolanin’s picture

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

did - manual testing of the 3 scenarios all behaves as expected.

cpliakas’s picture

cpliakas CreditAttribution: cpliakas commented

#6: 1791338-6.patch queued for re-testing.

cpliakas’s picture

cpliakas CreditAttribution: cpliakas commented

I can't apply the patch to test.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 1791338-6.patch, failed testing.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Status: Needs work » Needs review
FileSize
1791338-12.patch4.27 KB

re-rolled. Intervening commit made the 1st hunk not apply.

Nick_vh’s picture

Nick_vh
he/him
Location Ghent
CreditAttribution: Nick_vh commented
FileSize
1791338-12.patch4.27 KB
pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
FileSize
1791338-13.patch4.39 KB

Removed a change I shouldn't have.

Nick_vh’s picture

Nick_vh
he/him
Location Ghent
CreditAttribution: Nick_vh commented
Status: Needs review » Needs work 
+++ b/acquia_search/acquia_search.moduleundefined
@@ -84,13 +84,13 @@ function acquia_search_enable_acquia_solr_environment() {
-    // Make sure apachesolr search is the default search module.
-    variable_set('search_default_module', 'apachesolr_search');

this should not be removed?

cpliakas’s picture

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

this should not be removed?

It shouldn't. Makes me wonder why the testbot didn't pick this up since I wrote tests for this. Might have to revisit that as well.

EDIT: NM, it did pick it up for the patch in #12. Go test bot!

pwolanin’s picture

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

It did pick it up - that's why #12 fails.

In #14 it's just moved to the bottom of the function - I think it makes more sense to set it after the env is fully set up.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Status: Needs work » Fixed

committed. needs to be backported to acquia_search 6.x-3.x:

#1791638: Copy the index_bundles from the previous default environment to the new acquia_search environment

cpliakas’s picture

cpliakas CreditAttribution: cpliakas commented

Adding two related testing tasks that shouldn't hold up any releases.

#1791646: Add tests for copying the index_bundles from the previous default environment to the new acquia_search environment
#1791642: Add tests for the expected behavior when the Acquia Search module is disabled

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Status: Fixed » Needs review
FileSize
1791338-19-fu.patch705 bytes

Actually, looking at the code it seems like there is no reason to change the default environment if it points to a cloned env.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
FileSize
1791338-20-fu.patch1.19 KB

Hmm, I'm not sure why that prior code was working - it seems like it was setting bundles for the wrong env ID.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
FileSize
1791338-21-fu.patch1.92 KB

Maybe safer, API-wise, to delete the env after we check whether it's the default?

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Status: Needs review » Fixed

committed

Status: Fixed » Closed (fixed)

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