Allow to disable schema emulation on SQLite

[mvorisek]

Q	A
Type	bug
Fixed issues	#5517

Summary

Needed for atk4/data#1033

[mvorisek]

Guys, can this PR be merged as is as no tests is failing after the "eat prefixed tables" hack from 2011 has been removed? Or what test you want me to write?

[derrabus]

Looks like a duplicate of #4804.

[mvorisek]

Yes, it was fixed as part of #4804. Can it be backported to DBAL 3.3.x or to 3.4.x at least?

[derrabus]

If you find a backwards-compatible way to do so, we can probably discuss it. But this PR in its current form is a BC break unfortunately.

[mvorisek]

I will update this PR to move the str_replace('.', '__', ...) into protected method, so developer can override it. And it will stay backwards-compatible/unchanged in DBAL 3.x /w empty merge commit to 4.x.

[mvorisek]

done

[derrabus]

LGTM. @morozov, what do you think?

[mvorisek]

@derrabus is there anything againt to land this into 3.3.x? It is a kind of a bugfix.

[derrabus]

You're creating a new extension point which is beyond what a bugfix should do imho.

[mvorisek]

I am fixing "unhacked table name" support (see https://github.com/doctrine/dbal/pull/5517/files#r922818649) in BC way.

As long as the user does not override SqlitePlatform class and not implement emulateSchemaInTableName method in it, which is super unlikely, there is no single BC break.

[derrabus]

there is no single BC break

I'm not saying that there is one, am I? Why does it matter so much to you that we rush this into a bugfix release?

[mvorisek]

In atk4/data framework, I need to support working with multiple Sqlite databases. Without this PR, I would have to override and copy paste the fixed impl. in every method of SqlitePlatform class that uses the "eat prefixed tables" hack.

[morozov]

Introducing a protected method to allow overriding it to disable its functionality looks like a hack. If we want this behavior to be configurable, we need to add an API, e.g.

dbal/src/Platforms/PostgreSQLPlatform.php

Lines 73 to 76 in 25c27dd

	public function setUseBooleanTrueFalseStrings($flag)
	{
	$this->useBooleanTrueFalseStrings = (bool) $flag;
	}

The pull request is missing a description (as well as the linked issue) and tests.

As for whether to backport it to 3.3.x, most likely, 3.4.0 and 3.3.8 will be released together, so there's no point in backporting the patch IMO.

The base branch was changed.

[mvorisek]

I will add the option as in PostgreSQLPlatform. And a test /w ATTACH if the no other SqlitePlatform fixes will be needed.

About the targeting - if 3.4 will be out soon, it seems ok.

[derrabus]

The name of the new switch should reflect the feature that we're about to remove.

That being said, should that switch affect the return value of canEmulateSchemas() as well?

[mvorisek]

The name of the new switch should reflect the feature that we're about to remove.

That being said, should that switch affect the return value of canEmulateSchemas() as well?

Good point, I think yes to be consistent with #4804

[morozov]

Please squash the commits before the merge. The history of code changes in the feature branch isn't necessary to be stored in the repository history.

[mvorisek]

Feedback addressed and squashed.