r68127 MediaWiki - Code Review archive

Repository:	MediaWiki
Revision:	< r68126‎ \| r68127 \| r68128 >
Date:	18:26, 16 June 2010
Author:	mah
Status:	resolved (Comments)
Tags:
Comment:	* Create new DatabaseSqlite::checkForEnabledSearch() and use it from SearchSqlite::fulltextSearchSupported() instead of querying the updatelog table. This was the only use of updatelog under the non-maint code. * Add the searchindex table to the list of tables to preserve for testing. * Adapt SearchEngineTest to work with Sqlite. * Add fulltext setup for SQLite to the new installer code. * TODO: SqliteInstaller::setupSearchIndex() should not be using addHtml()
Modified paths:	/trunk/phase3/includes/db/DatabaseSqlite.php (modified) (history) /trunk/phase3/includes/installer/Installer.i18n.php (modified) (history) /trunk/phase3/includes/installer/SqliteInstaller.php (modified) (history) /trunk/phase3/includes/search/SearchSqlite.php (modified) (history) /trunk/phase3/maintenance/tests/MediaWikiParserTest.php (modified) (history) /trunk/phase3/maintenance/tests/SearchEngineTest.php (modified) (history)

Diff [purge]

Index: trunk/phase3/maintenance/tests/SearchEngineTest.php
—	—	@@ -14,7 +14,7 @@
15	15
16	16	function insertSearchData() {
17	17	if ( $this->pageExists( 'Not_Main_Page' ) ) {
18		~~- return;~~
	18	+ return;
19	19	}
20	20	$this->insertPage( "Not_Main_Page", "This is not a main page", 0 );
21	21	$this->insertPage( 'Talk:Not_Main_Page', 'This is not a talk page to the main page, see [[smithee]]', 1 );
—	—	@@ -45,7 +45,11 @@
46	46	}
47	47
48	48	function fetchIds( $results ) {
49		~~- if ( $this->db->getType() !== 'mysql' ) $this->markTestSkipped( "MySQL only" );~~
	49	+ $this->assertTrue( is_object( $results ) );
	50	+
	51	+ if ( $this->db->getType() !== 'mysql' && $this->db->getType() !== 'sqlite' ) {
	52	+ $this->markTestSkipped( "MySQL or SQLite only" );
	53	+ }
50	54	$matches = array();
51	55	while ( $row = $results->next() ) {
52	56	$matches[] = $row->getTitle()->getPrefixedText();
Index: trunk/phase3/maintenance/tests/MediaWikiParserTest.php
—	—	@@ -28,6 +28,7 @@
29	29	$tables[] = 'logging';
30	30	$tables[] = 'updatelog';
31	31	$tables[] = 'iwlinks';
	32	+ $tables[] = 'searchindex';
32	33	return true;
33	34	}
34	35
Index: trunk/phase3/includes/search/SearchSqlite.php
—	—	@@ -26,9 +26,6 @@
27	27	* @ingroup Search
28	28	*/
29	29	class SearchSqlite extends SearchEngine {
30		~~- // Cached because SearchUpdate keeps recreating our class~~
31		~~- private static $fulltextSupported = null;~~
32		-
33	30	/**
34	31	* Creates an instance of this class
35	32	* @param $db DatabaseSqlite: database object
—	—	@@ -42,18 +39,11 @@
43	40	* @return Boolean
44	41	*/
45	42	function fulltextSearchSupported() {
46		~~- if ( self::$fulltextSupported === null ) {~~
47		~~- self::$fulltextSupported = $this->db->selectField(~~
48		~~- 'updatelog',~~
49		~~- 'ul_key',~~
50		~~- array( 'ul_key' => 'fts3' ),~~
51		~~- __METHOD__ ) !== false;~~
52		~~- }~~
53		~~- return self::$fulltextSupported;~~
	43	+ return $this->db->checkForEnabledSearch();
54	44	}
55	45
56		- /**
57		~~- * Parse the user's query and transform it into an SQL fragment which will~~
	46	+ /**
	47	+ * Parse the user's query and transform it into an SQL fragment which will
58	48	* become part of a WHERE clause
59	49	*/
60	50	function parseQuery( $filteredText, $fulltext ) {
—	—	@@ -67,7 +57,7 @@
68	58	$filteredText, $m, PREG_SET_ORDER ) ) {
69	59	foreach( $m as $bits ) {
70	60	@list( /* all */, $modifier, $term, $nonQuoted, $wildcard ) = $bits;
71		-
	61	+
72	62	if( $nonQuoted != '' ) {
73	63	$term = $nonQuoted;
74	64	$quote = '';
—	—	@@ -86,7 +76,7 @@
87	77	} else {
88	78	$variants = array( $term );
89	79	}
90		-
	80	+
91	81	// The low-level search index does some processing on input to work
92	82	// around problems with minimum lengths and encoding in MySQL's
93	83	// fulltext engine.
—	—	@@ -94,12 +84,12 @@
95	85	$strippedVariants = array_map(
96	86	array( $wgContLang, 'normalizeForSearch' ),
97	87	$variants );
98		-
	88	+
99	89	// Some languages such as Chinese force all variants to a canonical
100	90	// form when stripping to the low-level search index, so to be sure
101	91	// let's check our variants list for unique items after stripping.
102	92	$strippedVariants = array_unique( $strippedVariants );
103		-
	93	+
104	94	$searchon .= $modifier;
105	95	if( count( $strippedVariants) > 1 )
106	96	$searchon .= '(';
—	—	@@ -114,7 +104,7 @@
115	105	}
116	106	if( count( $strippedVariants) > 1 )
117	107	$searchon .= ')';
118		-
	108	+
119	109	// Match individual terms or quoted phrase in result highlighting...
120	110	// Note that variants will be introduced in a later stage for highlighting!
121	111	$regexp = $this->regexTerm( $term, $wildcard );
—	—	@@ -129,10 +119,10 @@
130	120	$field = $this->getIndexField( $fulltext );
131	121	return " $field MATCH '$searchon' ";
132	122	}
133		-
	123	+
134	124	function regexTerm( $string, $wildcard ) {
135	125	global $wgContLang;
136		-
	126	+
137	127	$regex = preg_quote( $string, '/' );
138	128	if( $wgContLang->hasWordBreaks() ) {
139	129	if( $wildcard ) {
—	—	@@ -172,7 +162,7 @@
173	163	function searchTitle( $term ) {
174	164	return $this->searchInternal( $term, false );
175	165	}
176		-
	166	+
177	167	protected function searchInternal( $term, $fulltext ) {
178	168	global $wgCountTotalSearchHits, $wgContLang;
179	169
—	—	@@ -182,7 +172,7 @@
183	173
184	174	$filteredTerm = $this->filter( $wgContLang->lc( $term ) );
185	175	$resultSet = $this->db->query( $this->getQuery( $filteredTerm, $fulltext ) );
186		-
	176	+
187	177	$total = null;
188	178	if( $wgCountTotalSearchHits ) {
189	179	$totalResult = $this->db->query( $this->getCountQuery( $filteredTerm, $fulltext ) );
—	—	@@ -192,7 +182,7 @@
193	183	}
194	184	$totalResult->free();
195	185	}
196		-
	186	+
197	187	return new SqliteSearchResultSet( $resultSet, $this->searchTerms, $total );
198	188	}
199	189
—	—	@@ -226,7 +216,7 @@
227	217
228	218	/**
229	219	* Returns a query with limit for number of results set.
230		~~- * @param $sql String:~~
	220	+ * @param $sql String:
231	221	* @return String
232	222	*/
233	223	function limitResult( $sql ) {
—	—	@@ -246,7 +236,7 @@
247	237	$this->queryNamespaces()
248	238	);
249	239	}
250		-
	240	+
251	241	/**
252	242	* Picks which field to index on, depending on what type of query.
253	243	* @param $fulltext Boolean
—	—	@@ -300,7 +290,7 @@
301	291	$dbw = wfGetDB( DB_MASTER );
302	292
303	293	$dbw->delete( 'searchindex', array( 'rowid' => $id ), __METHOD__ );
304		-
	294	+
305	295	$dbw->insert( 'searchindex',
306	296	array(
307	297	'rowid' => $id,
Index: trunk/phase3/includes/db/DatabaseSqlite.php
—	—	@@ -12,6 +12,8 @@
13	13	*/
14	14	class DatabaseSqlite extends DatabaseBase {
15	15
	16	+ private static $fulltextEnabled = null;
	17	+
16	18	var $mAffectedRows;
17	19	var $mLastResult;
18	20	var $mDatabaseFile;
—	—	@@ -112,12 +114,29 @@
113	115	}
114	116
115	117	/**
	118	+ * Check if the searchindext table is FTS enabled.
	119	+ * @returns false if not enabled.
	120	+ */
	121	+ function checkForEnabledSearch() {
	122	+ if ( self::$fulltextEnabled === null ) {
	123	+ self::$fulltextEnabled = false;
	124	+ $res = $this->query( "SELECT sql FROM sqlite_master WHERE tbl_name = 'searchindex'", __METHOD__ );
	125	+ if ( $res ) {
	126	+ $row = $res->fetchRow();
	127	+ self::$fulltextEnabled = stristr($row['sql'], 'fts' ) !== false;
	128	+ }
	129	+ }
	130	+ return self::$fulltextEnabled;
	131	+ }
	132	+
	133	+ /**
116	134	* Returns version of currently supported SQLite fulltext search module or false if none present.
117	135	* @return String
118	136	*/
119	137	function getFulltextSearchModule() {
120	138	$table = 'dummy_search_test';
121	139	$this->query( "DROP TABLE IF EXISTS $table", __METHOD__ );
	140	+
122	141	if ( $this->query( "CREATE VIRTUAL TABLE $table USING FTS3(dummy_field)", __METHOD__, true ) ) {
123	142	$this->query( "DROP TABLE IF EXISTS $table", __METHOD__ );
124	143	return 'FTS3';
—	—	@@ -332,7 +351,7 @@
333	352
334	353	function replace( $table, $uniqueIndexes, $rows, $fname = 'DatabaseSqlite::replace' ) {
335	354	if ( !count( $rows ) ) return true;
336		-
	355	+
337	356	# SQLite can't handle multi-row replaces, so divide up into multiple single-row queries
338	357	if ( isset( $rows[0] ) && is_array( $rows[0] ) ) {
339	358	$ret = true;
—	—	@@ -498,7 +517,7 @@
499	518	if ( !$f ) {
500	519	dieout( "Could not find the interwiki.sql file." );
501	520	}
502		-
	521	+
503	522	$sql = "INSERT INTO interwiki(iw_prefix,iw_url,iw_local) VALUES ";
504	523	while ( !feof( $f ) ) {
505	524	$line = fgets( $f, 1024 );
—	—	@@ -507,7 +526,7 @@
508	527	$this->query( "$sql $matches[1],$matches[2])" );
509	528	}
510	529	}
511		-
	530	+
512	531	public function getSearchEngine() {
513	532	return "SearchSqlite";
514	533	}
—	—	@@ -627,7 +646,7 @@
628	647	return true;
629	648	}
630	649
631		~~- # isKey(), isMultipleKey() not implemented, MySQL-specific concept.~~
	650	+ # isKey(), isMultipleKey() not implemented, MySQL-specific concept.
632	651	# Suggest removal from base class [TS]
633	652
634	653	function type() {
Index: trunk/phase3/includes/installer/Installer.i18n.php
—	—	@@ -257,6 +257,9 @@
258	258	Check the data directory and database name below and try again.',
259	259	'config-sqlite-readonly' => 'File <code>$1</code> is not writeable.',
260	260	'config-sqlite-cant-create-db' => 'Could not create database file <code>$1</code>.',
	261	+ 'config-sqlite-fts3-downgrade' => 'PHP is missing FTS3 support, downgrading tables',
	262	+ 'config-sqlite-fts3-add' => 'Adding FTS3 search capabilities',
	263	+ 'config-sqlite-fts3-ok' => 'Fulltext search table appears to be in order',
261	264	'config-can-upgrade' => "There are MediaWiki tables in this database.
262	265	To upgrade them to MediaWiki $1, click '''Continue'''.",
263	266	'config-upgrade-done' => "Upgrade complete.
Index: trunk/phase3/includes/installer/SqliteInstaller.php
—	—	@@ -16,10 +16,10 @@
17	17
18	18	function getGlobalDefaults() {
19	19	if ( isset( $_SERVER['DOCUMENT_ROOT'] ) ) {
20		~~- $path = str_replace(~~
21		~~- array( '/', '\\' ),~~
22		~~- DIRECTORY_SEPARATOR,~~
23		~~- dirname( $_SERVER['DOCUMENT_ROOT'] ) . '/data'~~
	20	+ $path = str_replace(
	21	+ array( '/', '\\' ),
	22	+ DIRECTORY_SEPARATOR,
	23	+ dirname( $_SERVER['DOCUMENT_ROOT'] ) . '/data'
24	24	);
25	25	return array( 'wgSQLiteDataDir' => $path );
26	26	} else {
—	—	@@ -159,10 +159,33 @@
160	160	$this->db->reportQueryError( $err, 0, $sql, __FUNCTION__ );
161	161	}
162	162	//@todo set up searchindex
	163	+ $this->setupSearchIndex();
163	164	// Create default interwikis
164	165	return Status::newGood();
165	166	}
166	167
	168	+ function setupSearchIndex() {
	169	+ global $IP;
	170	+
	171	+ $module = $this->db->getFulltextSearchModule();
	172	+ $fts3tTable = $this->db->checkForEnabledSearch();
	173	+ if ( $fts3tTable && !$module ) {
	174	+ $this->parent->output->addHtml
	175	+ ( wfMsgHtml( 'word-separator' ) . wfMsgHtml( 'config-sqlite-fts3-downgrade' ) . wfMsgHtml( 'ellipsis' ) );
	176	+ $this->parent->output->flush();
	177	+ $this->db->sourceFile( "$IP/maintenance/sqlite/archives/searchindex-no-fts.sql" );
	178	+ } elseif ( !$fts3tTable && $module == 'FTS3' ) {
	179	+ $this->parent->output->addHtml
	180	+ ( wfMsgHtml( 'word-separator' ) . wfMsgHtml( 'config-sqlite-fts3-add' ) . wfMsg( 'ellipsis' ) );
	181	+ $this->parent->output->flush();
	182	+ $this->db->sourceFile( "$IP/maintenance/sqlite/archives/searchindex-fts3.sql" );
	183	+ } else {
	184	+ $this->parent->output->addHtml
	185	+ ( wfMsgHtml( 'word-separator' ) . wfMsgHtml( 'config-sqlite-fts3-ok' ) . wfMsgHtml( 'ellipsis' ) );
	186	+ $this->parent->output->flush();
	187	+ }
	188	+ }
	189	+
167	190	function doUpgrade() {
168	191	global $wgDatabase;
169	192	LBFactory::enableBackend();

Follow-up revisions

Revision	Commit summary	Author	Date
r75821	Follow-up r68127: honor table prefix, perverted though it may seem on SQLite.	maxsem	19:51, 1 November 2010

Comments

#Comment by Ævar Arnfjörð Bjarmason (talk | contribs) 15:31, 7 July 2010

The $this->db->sourceFile( "$IP/maintenance/sqlite/archives/searchindex-fts3.sql" ); part won't indicate what failed about sourcing the file.

This is what I get on my installation:

Creating tables...

Adding FTS3 search capabilities
Failed

You should catch the specific error that the database reports and present it to the user. I can't find what's wrong with it, sourcing maintenance/sqlite/archives/searchindex-fts3.sql manually works.

#Comment by MarkAHershberger (talk | contribs) 05:07, 8 July 2010

This particular code works.

What is broken is that there is no way to convey information other than failure or success through the Status object. You can have (until recently) the ability to return "non-fatal" errors, but there isn't a way to display this information to the user (at least, in the installer).

The message is informational, but the WebInstaller code has not been updated yet to show these sorts of messages without calling them fatal.

(If installation does actually fail and a LocalSettings file isn't created, then the above doesn't apply.)

#Comment by 😂 (talk | contribs) 18:44, 9 July 2010

The situation for reporting Status objects should be a bit better as of r69097, r69098 and r69103. Each step is given a chance to do callbacks on the Status object returned from the installation step. WebInstaller will output appropriate messages for OK/Warning/Fatal now.

#Comment by Ævar Arnfjörð Bjarmason (talk | contribs) 18:48, 9 July 2010

Right, you can now output informative messages.

I couldn't find any way to make sourceFile output an error message, just a boolean failure/success code. The SQLite library itself will give you detailed errors, so those are being hidden away somewhere.

#Comment by Nikerabbit (talk | contribs) 05:15, 4 August 2010

+       'config-sqlite-fts3-downgrade'    => 'PHP is missing FTS3 support, downgrading tables',
+       'config-sqlite-fts3-add'          => 'Adding FTS3 search capabilities',
+       'config-sqlite-fts3-ok'           => 'Fulltext search table appears to be in order',

Should be reworded to be more precise and include explanatory terms like fulltext search so the user knows what is going on. Downgrading tables should perhaps be replaced with "removing support for fulltext (search|tables)" or so.

Status & tagging log

00:50, 1 November 2010 😂 (talk | contribs) changed the status of r68127 [removed: new added: resolved]
00:56, 15 July 2010 MarkAHershberger (talk | contribs) changed the status of r68127 [removed: fixme added: new]
15:31, 7 July 2010 Ævar Arnfjörð Bjarmason (talk | contribs) changed the status of r68127 [removed: new added: fixme]