Skip to content
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

Disabled enhanced-search does not disable ElasticPress #48

Open
jazzsequence opened this issue Dec 18, 2019 · 7 comments · May be fixed by #75
Open

Disabled enhanced-search does not disable ElasticPress #48

jazzsequence opened this issue Dec 18, 2019 · 7 comments · May be fixed by #75
Labels
bug Existing functionality isn't behaving as expected should have Should be done, medium priority for now

Comments

@jazzsequence
Copy link
Contributor

jazzsequence commented Dec 18, 2019

When enhanced-search is disabled at a module (or environment) level, ElasticPress is not actually disabled and displays a not configured warning in the dashboard if an ElasticSearch server has not been configured.

A quick look at this module suggests that this is because the module is being registered as search rather than enhanced-search: https://github.com/humanmade/altis-enhanced-search/blob/master/load.php#L14

Workaround is to define altis.modules.search.enabled instead of altis.modules.enhanced-search.enabled.

Long term solution may require adding an alias (per @roborourke slack convo) for backwards compatibility.

@jazzsequence jazzsequence added bug Existing functionality isn't behaving as expected good first issue Good for newcomers labels Dec 18, 2019
@roborourke
Copy link
Contributor

We don’t actually have the example docs for how to disable this module and that the key is actually search. We were going to rename this module to search but that fell off the radar. We can do that properly in the next version, but search will be the correct name.

We could rename this repo any time which makes one less point of confusion

@rmccue
Copy link
Member

rmccue commented Dec 19, 2019

We'll have to rename the Packagist package as well I imagine, which will need a bit of work?

@roborourke
Copy link
Contributor

@rmccue it's not super terrible, we'd essentially update the name in composer.json, update the name of this repo, perhaps add a "suggest" section to the composer.json and then resubmit on packagist with the new repo name and archive the old one.

@rmccue
Copy link
Member

rmccue commented Dec 19, 2019

For the LTS policy, I think we'd need to keep it unarchived at least until v3 is out of LTS?

@roborourke
Copy link
Contributor

Derp, yeah you're right. Not sure how well packagist copes with 2 packages driven from the same repo.

@roborourke roborourke removed the good first issue Good for newcomers label Dec 19, 2019
@roborourke
Copy link
Contributor

Removed the good first issue label, aliasing module names is actually gonna be quite fiddly and possibly to be avoided. I've added docs now for how to switch it off, and the longer term solution is to rename the repo & package.

@roborourke
Copy link
Contributor

In the meantime I've added some criteria to humanmade/altis-core#21 to help with this issue so there will at least be some basic validation.

@roborourke roborourke linked a pull request Jun 9, 2020 that will close this issue
@PriyaBhatia0210 PriyaBhatia0210 added the should have Should be done, medium priority for now label Mar 30, 2021
@yumito yumito removed the cross-team label Nov 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Existing functionality isn't behaving as expected should have Should be done, medium priority for now
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants