-
-
Notifications
You must be signed in to change notification settings - Fork 994
[feature/sphinx fulltext search] #865
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
Conversation
phpBB/includes/functions_sphinx.php
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I can see, you're not actually using $i?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
thanks @callumacrae for commenting, will make the required changes. |
phpBB/docs/sphinx.sample.conf
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should not be avatar salt probably.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about generating a new unique value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose of having a unique value here in the first place?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This value should be unique for a particular web server as sphinx may be being used for indexing apart from phpbb.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is configuration from this file merged with some other configuration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No it is not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then how would sphinx be used for indexing anything other than the phpbb installation that generated the configuration file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unique key will ensure sphinx server can be used for multiple phpBB installations. I will generate a new key instead of using salt.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These properties should gain docblocks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a PR to make db classes autoloadable that might affect this line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cross link: #908
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Support arbitrary hosts?
|
sphinxapi.php (last released version, or 0.9.8) should be included before merge. |
|
Auth should be checked everywhere. |
Use // for two liners in comments. PHPBB3-10946
Add a new line after break. Change docblocks to be more informative. PHPBB3-10946
|
Have fixed and tested everything. |
phpBB/language/en/acp/search.php
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change default port to 9312.
|
Please include sphinxapi.php back. |
Uses 9312 instead of 3312 as default port for searchd to listen on according to latest sphinx documentation. Use filename sphinxapi.php instead of old one. PHPBB3-10946
Removes unused property $word_length PHPBB3-10946
$this->auth replaces $auth as at other occurences of auth. PHPBB3-10946
|
Fixed default port and added sphinxapi.php. |
…' into develop * dhruvgoel92/feature/sphinx-fulltext-search: (57 commits) [feature/sphinx-fulltext-search] add sphinx to Authors file [feature/sphinx-fulltext-search] add sphinxapi.php file [feature/sphinx-fulltext-search] fix auth bug [feature/sphinx-fulltext-search] remove unused property [feature/sphinx-fulltext-search] use 9312 as default port [feature/sphinx-fulltext-search] fix language of host config [feature/sphinx-fulltext-search] fix sphinx for arbitary host [feature/sphinx-fulltext-search] coding changes acc to phbb conventions [feature/sphinx-fulltext-search] fixing comments [feature/sphinx-fulltext-search] add trailing slash in language [feature/sphinx-fulltext-search] improve port option [feature/sphinx-fulltext-search] remove stopwords and config path [feature/sphinx-fulltext-search] makes sql host configurable [feature/sphinx-fulltext-search] use readonly instead of disabled [feature/sphinx-fulltext-search] fix language keys' typo [feature/sphinx-fulltext-search] remove note from db_tools [feature/sphinx-fulltext-search] add support for postgres [feature/sphinx-fulltext-search] add pgsql functionality [feature/sphinx-fulltext-search] use Update in sphinx query [feature/sphinx-fulltext-search] use CASE instead of IF ...
Integrates sphinx fulltext search backend into the phpbb core.
http://tracker.phpbb.com/browse/PHPBB3-10946
PHPBB3-10946