Skip to content

Conversation

@dhruvgoel92
Copy link
Contributor

Integrates sphinx fulltext search backend into the phpbb core.

http://tracker.phpbb.com/browse/PHPBB3-10946

PHPBB3-10946

@travisbot
Copy link

This pull request passes (merged 1d01bc2d into 85ea062).

Copy link
Contributor

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?

Choose a reason for hiding this comment

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

@dhruvgoel92
Copy link
Contributor Author

thanks @callumacrae for commenting, will make the required changes.

Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No it is not.

Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cross link: #908

Copy link
Contributor

Choose a reason for hiding this comment

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

Support arbitrary hosts?

@p
Copy link
Contributor

p commented Jul 26, 2012

sphinxapi.php (last released version, or 0.9.8) should be included before merge.

@p
Copy link
Contributor

p commented Jul 26, 2012

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
@travisbot
Copy link

This pull request passes (merged a6b5b27 into d1e5686).

@dhruvgoel92
Copy link
Contributor Author

Have fixed and tested everything.

@travisbot
Copy link

This pull request passes (merged ea4b650d into d1e5686).

Copy link
Contributor

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.

@p
Copy link
Contributor

p commented Jul 27, 2012

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
@dhruvgoel92
Copy link
Contributor Author

Fixed default port and added sphinxapi.php.

@travisbot
Copy link

This pull request passes (merged f172928 into d1e5686).

p added a commit to p/phpbb3 that referenced this pull request Jul 28, 2012
…' 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
  ...
@p p merged commit f172928 into phpbb:develop Jul 28, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants