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

[Platform.sh] Only enable Solr if parameter 'search_engine' is 'solr' #540

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ChrisMoutsos
Copy link

If there is a Platform.sh Solr relationship, but the project config is not set to use the "solr" search engine (i.e. it's set to "legacy", or not set at all), then the Solr params shouldn't be set here.

Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution. Makes sense in general.
However please create a JIRA ticket and prefix the PR title with it, followed by a comma (:) and a short description of the change (ideally in past simple as it's gonna become history after merge).
If this causes some sort of a visible bug and is reproducible on a clean installation, then some steps to reproduce should be added. I'm assuming that it happens when you configure P.sh to use Solr, but set legacy, right? Does it make the system crash or just index data incorrectly?

As for the change itself:

@@ -105,7 +105,7 @@
}
}

if (isset($relationships['solr'])) {
if (isset($relationships['solr']) && ($_SERVER['SEARCH_ENGINE'] ?? false) === 'solr') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if a defined parameter is a string, then setting bool as a fallback doesn't make any sense, even if it works

Suggested change
if (isset($relationships['solr']) && ($_SERVER['SEARCH_ENGINE'] ?? false) === 'solr') {
if (isset($relationships['solr']) && ($_SERVER['SEARCH_ENGINE'] ?? '') === 'solr') {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants