Use Deprecation::triggerIfCalledFromOutside in listTableDetails

[Aweptimum]

Q	A
Type	bug/feature/improvement
Fixed issues	#6085

Summary

Except for the Sqlite platform (which already used triggerIfCalledFromOutside), replaced the use of Deprecation::trigger with Deprecation::triggerIfCalledFromOutside in the SchemaManagers

[Aweptimum]

Had to force-push because my editor didn't save the change in MySqlSchemaManager

[greg0ire]

The commit message should explain the reason behind this change.

[Aweptimum]

For the force-push?

[greg0ire]

The second commit should be fixed up into the first one, but that's a detail. The first commit should explain why you are doing this (because these methods are still called internally, right?).

[Aweptimum]

Got it, I've amended the commit with a more detailed explanation.

[greg0ire]

Thanks @Aweptimum !

[VincentLanglet]

HI @Aweptimum, I just found the PR and I'm not sure it will solve all the deprecations:

Currently I get

User Deprecated: Doctrine\DBAL\Schema\MySQLSchemaManager::listTableDetails is deprecated. Use introspectTable() instead. (MySQLSchemaManager.php:76 called by AbstractSchemaManager.php:614, https://github.com/doctrine/dbal/pull/5595, package doctrine/dbal)

and

User Deprecated: Passing $database to AbstractSchemaManager::listTableColumns() is deprecated. (AbstractSchemaManager.php:245 called by MySQLSchemaManager.php:91, https://github.com/doctrine/dbal/issues/5284, package doctrine/dbal)

This PR will solve the first one, but there will still be the second one.

The issue is that introspectTable(string $name) is calling listTableDetails($name) which is calling listTableColumns($name) but this trigger a deprecation when $name is not null.

Do you or @greg0ire know how to solve the deprecation ?

[Aweptimum]

It seems each concrete schema managers' listTableDetails calls the protected AbstractSchemaManager->doListTableDetails, which does pass the deprecated $database parameter to AbstractSchemaManager->listTableColumns, but the further complication is that each sub-type overrides listTableColumns to use the protected AbstractSchemaManager->doListTableColumns($table, $database). Which has the same deprecation trigger as the public listTableColumns.

I understand the deprecations being in the public methods, but not in the protected methods. The solution might be to move the deprecation trigger out of doListTableColumns and up to each instance of listTableColumns in the schema managers. This would be consistent with the AbstractSchemaManager as that is where its deprecation trigger is.

But I'm very ignorant of the intentions here, there's really only this issue to go off of: #5284
It seems to me that morozov might have took issue with the inconsistencies of the public api, maybe not so much the internal one. However, if the change really was meant to target the internal API as well, then another solution would be needed.

I would wait for gregoire's input, but it is probably worth a new issue

[greg0ire]

Yes, a separate issue please, with as many github permalinks as possible please. I don't want to have to click around 36 files to understand this.

this trigger a deprecation when $name is not null.

🤔 no it does not:

dbal/src/Schema/AbstractSchemaManager.php

Lines 213 to 223 in 96d5a70

	public function listTableColumns($table, $database = null)
	{
	if ($database === null) {
	$database = $this->getDatabase(__METHOD__);
	} else {
	Deprecation::trigger(
	'doctrine/dbal',
	'https://github.com/doctrine/dbal/issues/5284',
	'Passing $database to AbstractSchemaManager::listTableColumns() is deprecated.',
	);
	}

Only when $database is not null.

[VincentLanglet]

this trigger a deprecation when $name is not null.

🤔 no it does not:

dbal/src/Schema/AbstractSchemaManager.php

Lines 213 to 223 in 96d5a70

	public function listTableColumns($table, $database = null)
	{
	if ($database === null) {
	$database = $this->getDatabase(__METHOD__);
	} else {
	Deprecation::trigger(
	'doctrine/dbal',
	'https://github.com/doctrine/dbal/issues/5284',
	'Passing $database to AbstractSchemaManager::listTableColumns() is deprecated.',
	);
	}

Only when $database is not null.

My bad, I was confuse ; still I think I found a deprecation to solve #6104 ^^