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

listTables() failing to use correct prefix #2210

Closed
MGatner opened this issue Sep 10, 2019 · 6 comments
Closed

listTables() failing to use correct prefix #2210

MGatner opened this issue Sep 10, 2019 · 6 comments

Comments

@MGatner
Copy link
Member

MGatner commented Sep 10, 2019

Linux, PHP 7.2, Latest develop branch, MySQLi

If I use $db->listTables(true) (for $constrainByPrefix) with a prefix that includes an _I get a SQL statement that doesn't work. Pseudo-code:

$db = db_connect(['DBPrefix' => 'dev_']);
$db->listTables(true);
echo (string)$db->getLastQuery();

I see that it is trying to escape the underscore with a bang (!):
SHOW TABLES FROM database LIKE 'dev!_%'

Tracing it back this is indeed what is set on the BaseConnection:

/**
 * ESCAPE character
 *
 * @var string
 */
public $likeEscapeChar = '!';

What am I missing? That doesn't work as an escape, should be \, right?

@MGatner
Copy link
Member Author

MGatner commented Sep 10, 2019

Postrge seems to provide an additional ESCAPE '!' to define the escape character:

return $sql . ' AND "table_name" LIKE \''
	. $this->escapeLikeString($this->DBPrefix) . "%' "
	. sprintf($this->likeEscapeStr, $this->likeEscapeChar);

But I don't think that works in MySQLi and it's trying to use ! regardless.

@MGatner
Copy link
Member Author

MGatner commented Sep 11, 2019

There are actually a couple problems here... The escape character is one, but I'm baffled why it isn't causing failures in #2211. The other issue is that listTables() doesn't check $constrainPrefix before returning a cached value. That should be a pretty easy fix though.

@michalsn
Copy link
Member

From what I can see this is a bug inherited from CI3.

The thing is that SHOW TABLES FROM query can't have custom escaping character in LIKE statement. So something like this won't work:

SHOW TABLES FROM `database` LIKE 'dev!_%' ESCAPE '!';

We can try to use something like this:

SELECT `table_name` FROM information_schema.tables WHERE `table_schema` = 'database' AND `table_name` LIKE 'dev!_%' ESCAPE '!';

But I'm not sure if users always have access to information_schema. Any feedback on this would be appreciated. If so, we can do just:

protected function _listTables(bool $prefixLimit = false): string
{
	$sql = 'SHOW TABLES FROM ' . $this->escapeIdentifiers($this->database);

	if ($prefixLimit !== false && $this->DBPrefix !== '')
	{
		$sql = "SELECT table_name FROM information_schema.tables WHERE table_schema = '" . $this->escapeIdentifiers($this->database) . "' AND table_name LIKE '" . $this->escapeLikeString($this->DBPrefix) . "%'" . sprintf($this->likeEscapeStr, $this->likeEscapeChar);
	}

	return $sql;
}

@MGatner
Copy link
Member Author

MGatner commented Sep 17, 2019

Ah that’s good info! I didn’t realize SHOW TABLES can’t take escape sequences but it makes sense as the cause. That means that there’s a deeper issue and the MySQLi driver shouldn’t be using escapeChar for some queries. We need a way to determine when that is and differentiate how to escape in light of that. SHOW TABLES FROM ‘database’ LIKE ‘prefix\_’ works fine so maybe we need two different escape functions?

Incidentally I think listTables() is the only function like this right now so it could probably be hard-coded into the driver but I want to make sure any future additions handle it correctly.

@jim-parry
Copy link
Contributor

Is this now fixed then, by #2229?

@MGatner
Copy link
Member Author

MGatner commented Sep 27, 2019

Yes! Closing.

@MGatner MGatner closed this as completed Sep 27, 2019
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 a pull request may close this issue.

3 participants