Add supported read isolation level to schema interface

[caddoo]

Description:

TiDB doesn't support READ_UNCOMMITTED, I've added this variance to the schema.

To make sure it works, a test has been added to the LogAggregator making sure the supportsUncommitted doesn't switch to false but instead true

Review

• Functional review done
• Potential edge cases thought about (behavior of the code with strange input, with strange internal state or possible interactions with other Matomo subsystems)
• Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
• Security review done
• Wording review done
• Code review done
• Tests were added if useful/possible
• Reviewed for breaking changes
• Developer changelog updated if needed
• Documentation added if needed
• Existing documentation updated if needed

[sgiehl]

Some general things are missing around the deprecation of setUncommitted method:

The deprecation needs to be added to our changelog:

matomo/CHANGELOG.md

Lines 13 to 16 in d712ca6

	## Deprecations
	The methods `Db::isOptimizeInnoDBSupported` and `Db::optimizeTables` have been deprecated. Use `Db\Schema::getInstance()->isOptimizeInnoDBSupported` and `Db\Schema::getInstance()->optimizeTables` instead

And it would be good to add a test that ensures we don't forget to remove the method with Matomo 6:

matomo/tests/PHPUnit/Unit/DeprecatedMethodsTest.php

Lines 90 to 93 in d712ca6

	$this->assertDeprecatedMethodIsRemovedInMatomo6('Piwik\Common', 'getRequestVar');
	$this->assertDeprecatedMethodIsRemovedInMatomo6('Piwik\Plugins\Overlay\API', 'getExcludedQueryParameters');
	$this->assertDeprecatedMethodIsRemovedInMatomo6('Piwik\Db', 'isOptimizeInnoDBSupported');
	$this->assertDeprecatedMethodIsRemovedInMatomo6('Piwik\Db', 'optimizeTables');

[sgiehl]

Guess it's discussable if the public db property should still be called supportsUncommitted, when it actually no longer holds that information.

[caddoo]

I agree it's name wrong, on one side as well I'm not exactly sure why it's exposed as public in the first place.

Would you suggest making it private and make this class less leaky, along with a proper deprecation message?

[sgiehl]

I guess it's exposed public and set for the DB class, so the check is only performed once per DB connection. As the property shouldn't be used by any plugin I would say we can simply rename it, or make it private and introduce methods to handle that correctly.
We could even consider to handle everything through DB class, instead of using the TransactionLevel class directly.

[caddoo]

Some general things are missing around the deprecation of setUncommitted method:

The deprecation needs to be added to our changelog:

matomo/CHANGELOG.md

Lines 13 to 16 in d712ca6

	## Deprecations
	The methods `Db::isOptimizeInnoDBSupported` and `Db::optimizeTables` have been deprecated. Use `Db\Schema::getInstance()->isOptimizeInnoDBSupported` and `Db\Schema::getInstance()->optimizeTables` instead

And it would be good to add a test that ensures we don't forget to remove the method with Matomo 6:

matomo/tests/PHPUnit/Unit/DeprecatedMethodsTest.php

Lines 90 to 93 in d712ca6

	$this->assertDeprecatedMethodIsRemovedInMatomo6('Piwik\Common', 'getRequestVar');
	$this->assertDeprecatedMethodIsRemovedInMatomo6('Piwik\Plugins\Overlay\API', 'getExcludedQueryParameters');
	$this->assertDeprecatedMethodIsRemovedInMatomo6('Piwik\Db', 'isOptimizeInnoDBSupported');
	$this->assertDeprecatedMethodIsRemovedInMatomo6('Piwik\Db', 'optimizeTables');

Thanks for this, I've done these changes now.