Support for backslashes in default values #2850

PowerKiKi · 2017-09-09T15:25:44Z

It is now possible to use \, or other escaped characters, in default
values for a column.

Previously this lead to a confusion when diffing actual and expected
schema leading to perpetual out of sync schema.

greg0ire · 2017-09-19T18:41:10Z

Please rebase on the latest master to fix your build

morozov · 2017-09-20T01:52:05Z

tests/Doctrine/Tests/DBAL/Functional/Schema/MySqlSchemaManagerTest.php

@@ -180,6 +180,21 @@ public function testDoesNotPropagateDefaultValuesForUnsupportedColumnTypes()
        $this->assertFalse($onlineTable->getColumn('def_blob_null')->getNotnull());
    }

+    public function testEscapedDefaultValueMustBePreserved()


Since it's a functional test and it doesn't check any platform-specific logic, I think we could move it to a new SchemaManagerTest which instead of instantiating the schema manager would take it from $conn->getSchemaManager().

This way, we could make sure this feature is portable and can be safely used on all supported platforms.

I agree with you, but I don't think I'll be able to test all platforms unfortunately. I might give it a try via Travis though, but that might be cumbersome. I'll see what I can do...

@PowerKiKi this is the right approach. You don't have to test other platforms yourself. I can run the updated tests on platforms not supported by Travis manually.

morozov · 2017-09-20T02:21:47Z

lib/Doctrine/DBAL/Platforms/MySqlPlatform.php

@@ -385,7 +385,7 @@ public function getListTableColumnsSQL($table, $database = null)
        }

        return "SELECT COLUMN_NAME AS Field, COLUMN_TYPE AS Type, IS_NULLABLE AS `Null`, ".
-               "COLUMN_KEY AS `Key`, COLUMN_DEFAULT AS `Default`, EXTRA AS Extra, COLUMN_COMMENT AS Comment, " .
+               "COLUMN_KEY AS `Key`, TRIM('\'' FROM QUOTE(COLUMN_DEFAULT)) AS `Default`, EXTRA AS Extra, COLUMN_COMMENT AS Comment, " .


Is the problem really in the introspection query? If I add the following assertion to the newly introduced test,

$this->_conn->executeUpdate('INSERT INTO string_escaped_default_value VALUES()'); $fetched = $this->_conn->executeQuery('SELECT def_string from string_escaped_default_value') ->fetchColumn(); $this->assertSame($value, $fetched);

it fails as:

1) Doctrine\Tests\DBAL\Functional\Schema\MySqlSchemaManagerTest::testEscapedDefaultValueMustBePreserved Failed asserting that two strings are identical. --- Expected +++ Actual @@ @@ -a\0a\'a"a +aa'a"a a -a a\Za\\a +a a�a\a

Which probably means that the actual default value is not what the tests defines and then retrieves.

The following script demonstrates that there's no escaping required when introspecting the shema:

mysql> CREATE TABLE test -> ( -> test VARCHAR(32) DEFAULT '\\\'\"' NULL -> ); Query OK, 0 rows affected (0.04 sec) mysql> mysql> INSERT INTO test VALUES (); Query OK, 1 row affected (0.01 sec) mysql> mysql> SELECT * -> FROM test; +------+ | test | +------+ | \'" | +------+ 1 row in set (0.00 sec) mysql> mysql> SELECT -> COLUMN_NAME AS Field, -> COLUMN_DEFAULT AS `Default` -> FROM information_schema.COLUMNS -> WHERE TABLE_SCHEMA = database() AND TABLE_NAME = 'test'; +-------+---------+ | Field | Default | +-------+---------+ | test | \'" | +-------+---------+ 1 row in set (0.01 sec)

I was under the impression that getListTableColumnsSQL() was supposed to return SQL that could be used directly, without any further escaping. And more specifically that they could be compared directly to the value coming from user-defined mapping via Comparator::diffTable(). Since that user-defined value must be escaped, the introspected value should also be escaped. That's why I chose to do the escaping in there.

But maybe we can see the issue from a different point of view and always diff un-escaped values ? so that means that the user-defined value coming from the mapping should be un-escaped ? But since escaping is platform specific where/how should we un-escaped user-defined values ?

I was under the impression that getListTableColumnsSQL() was supposed to return SQL that could be used directly, without any further escaping.

Designed this way, it would allow stored SQL injections, so no.

But maybe we can see the issue from a different point of view and always diff un-escaped values?

Yes, the values should be only escaped when building SQL containing them.

So that means that the user-defined value coming from the mapping should be un-escaped?

Yes, because escaping is platform-specific.

But since escaping is platform specific where/how should we un-escaped user-defined values?

Why are user-defined values escaped and which platform are they escaped for?

Why are user-defined values escaped?

Because if I want my default value to be Foo\Bar\Baz, I will have to escape the \ and thus writing something like @ORM\Column(options={"default" = "Foo\\Bar\\Baz"})).

I see that for a similar problem with quoted table names, that it was resolved in AbstractAsset::_setName() where quotes are detected and removed before comparison.

Should we adopt a similar strategy and modify Column::setDefault() to automatically un-escape strings ? If so should we somehow detect which strings need to be un-escaped ? or un-escape all of them and then introducing a breaking change ?

Another approach would be to write the mapping non-escaped, and let Doctrine do the escaping automatically in all cases. But that would directly contradict what is done for table names and would also introduce a breaking change. So I don't think it's the best way to solve that.

The more I think about this, the more I think we won't be able to fix it without introducing a potential breaking change. But maybe there is yet another way... ? Maybe the original way to escape introspected value is less breaking after all ?

Because if I want my default value to be Foo\Bar\Baz, I will have to escape the \ and thus writing something like @ORM\Column(options={"default" = "Foo\\Bar\\Baz"})).

It's the escaping required by the PHP string syntax, the value of the default does not contain escaped slashes when used in defining the schema:

echo "Foo\\Bar\\Baz", PHP_EOL; // Foo\Bar\Baz

I see that for a similar problem with quoted table names, that it was resolved in AbstractAsset::_setName() where quotes are detected and removed before comparison.

Should we adopt a similar strategy and modify Column::setDefault() to automatically un-escape strings? If so should we somehow detect which strings need to be un-escaped ? or un-escape all of them and then introducing a breaking change?

I don't know the implementation details of the above, but there shouldn't be any un-escaping in any layer. The un-escaping is only supposed to be done by the parser of the language in whose syntax the escaping is done. Like in my code example, the un-escaping is done by PHP.

Another approach would be to write the mapping non-escaped, and let Doctrine do the escaping automatically in all cases. But that would directly contradict what is done for table names and would also introduce a breaking change. So I don't think it's the best way to solve that.

This is the only right way how things should work. None values should be escaped anywhere else then before concatenation with SQL/HTML/whatever "program". Furthermore, in the SQL context, if a value is bound to a prepared statement, it must not be escaped because it's not parsed.

morozov

@PowerKiKi please see my comments.

PowerKiKi · 2017-09-21T07:17:09Z

It's the escaping required by the PHP string syntax.

This is incorrect, in my example I use the annotation which are plain text parsed by doctrine/annotations, not by PHP parser. So PHP escaping rules do not apply here. As a proof we can compare what happens with a single \ which are interpreted differently by PHP or MySQL:

echo "Double\\Single\Foo", PHP_EOL; // Single backslash will be preserved by PHP
// Double\Single\Foo

SELECT "Double\\Single\Foo"; -- Single backslash will be removed by MySQL
+------------------+
| Double\SingleFoo |
+------------------+

And then running vendor/bin/doctrine orm:schema-tool:update --dump-sql we can see that the SQL being executed is exactly as what I wrote. If the mapping is:

/**
 * @ORM\Column(nullable=true, options={"default" = "Double\\Single\Foo"}))
 */
private $name;

Then the SQL that will be executed is reported as:

ALTER TABLE my_table CHANGE name name VARCHAR(255) DEFAULT 'Double\\Single\Foo' NOT NULL;

Finally if forcing an update and then checking the actual schema in database with our standard introspection query, we can see that the default is indeed escaped the MySQL way, with the single single dash trimmed:

SELECT COLUMN_NAME AS Field, COLUMN_DEFAULT AS `Default` FROM information_schema.COLUMNS WHERE TABLE_SCHEMA = database() AND TABLE_NAME = 'my_table';
+-------+------------------+
| Field | Default          |
+-------+------------------+
| id    | NULL             |
| name  | Double\SingleFoo |
+-------+------------------+

So we can conclude that default values are given to MySQL exactly as written in the annotation, without being affected by PHP escape rules, and without any other form of escaping by doctrine. And we can also clearly confirm that the value in the mapping Double\\Single\Foo is not equals to the introspected value Double\SingleFoo. So they should not be compared without some kind of escaping/un-escaping beforehand. The question being on what side should we do that extra work ?

One more interesting observation is that SHOW CREATE TABLE my_table; will correctly return an escaped value which is ready to be used. That is precisely the behavior I tried to emulate with this PR:

SHOW CREATE TABLE my_table;
+----------+--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| Table    | Create Table                                                                                                                                                                                                             |
+----------+--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| my_table | CREATE TABLE `my_table` (
  `id` int(11) NOT NULL,
  `name` varchar(255) COLLATE utf8mb4_unicode_ci DEFAULT 'Double\\SingleFoo',
  PRIMARY KEY (`id`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci |
+----------+--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+

On a sidenote, it is currently possible do SQL injection with an annotation like:

/**
 * @ORM\Column(nullable=true, options={"default" = "'; DROP TABLE my_table; SELECT ''='"}))
 */
private $name;

PowerKiKi · 2017-09-21T07:46:33Z

shouldn't be any un-escaping in any layer

Entirely agree with that. I wouldn't even know how to implement it anyway.

None values should be escaped anywhere else then before concatenation with SQL/HTML/whatever "program".

Sounds fair to me. So we write annotations non-escaped, and Doctrine will escaped them when building SQL statements. It totally make senses actually.

But it differs from what is done for table names were doctrine never do any escaping/quoting and it is up to end-user to quote the table name if required. Because of that I was not sure if it was the right way to go forward and introduce an inconsistent behavior between table names and default values.

morozov · 2017-09-21T19:23:07Z

It's the escaping required by the PHP string syntax.

This is incorrect, in my example I use the annotation which are plain text parsed by doctrine/annotations, not by PHP parser.

@PowerKiKi you're right, thanks for pointing that out.

Currently, the default value is only quoted but not escaped when the column DDL is generated:

dbal/lib/Doctrine/DBAL/Platforms/AbstractPlatform.php

Line 2280 in 80f42dc

$default = " DEFAULT '".$field['default']."'";

First, I thought it was done to support default values which are expressions (like CURRENT_TIMESTAMP()) but they seem to be taken care of outside of that line. Furthermore, surrounded with quotes, an expression turns into a string literal anyways.

While default values are given to MySQL exactly as written in the annotation, I don't (at least at the moment) see why it's not the bug which should be fixed in the first place. Given that escaping is platform-specific, it's just impossible to create an escaped default value which would be handled correctly by all platforms, so this behavior shouldn't be relied on. And as you pointed out, it allows injecting SQL into annotations.

With that said, could you try adding escaping there and see if it breaks anything? It will definitely break existing metadata which has escaped special characters because the DBAL won't expect them to be escaped anymore.

PowerKiKi · 2017-09-22T04:57:49Z

I just forced push a new patch, which is way cleaner and supports all platforms.

I had a look to document the change, but it didn't feel right to add that to http://docs.doctrine-project.org/projects/doctrine-dbal/en/latest/reference/schema-representation.html#portable-options. So I suppose it should appear somewhere in UPGRADE.md, but I couldn't figure out where it should go. Any advice about that ?

morozov · 2017-09-22T17:40:14Z

I'd restore your previous test and add the lines I left earlier in comments. The reasons are:

Having the default value escaped is not a requirement of the DBAL itself, it's a requirement for integration with a given database platform, so it should be tested in a functional manner.
The DBAL is not the source of truth about the way strings should be escaped for a given platform. Thus, the test shouldn't have any assertions against the escaped strings. Otherwise, if the implementation is incorrect, and the test is written after implementation, it will pass even if the implementation is incorrect.
What's more important, is that a value specified in the column definition is indeed the value populated by default (covered by Support for backslashes in default values #2850 (comment)), and the same value is returned by introspecting the database schema (covered by 4dfc5c2#diff-921db63349be598a376afb1584ccc8b5R192).

I had a look to document the change, but it didn't feel right to add that to http://docs.doctrine-project.org/projects/doctrine-dbal/en/latest/reference/schema-representation.html#portable-options. So I suppose it should appear somewhere in UPGRADE.md, but I couldn't figure out where it should go. Any advice about that ?

I don't think the documentation needs to be updated as this patch doesn't invalidate the article. As for UPGRADE.md — it will depend on which release this patch gets in. I believe it could be accepted for 2.6 even if it's a minor BC break (the escaped values were not portable anyway). You can draft the notes for 2.6 and we could move them to a different release if needed.

Let's finalize the code changes first to see what exactly the upgrade procedure is.

belgattitude · 2017-09-27T12:20:43Z

@PowerKiKi

Coincidentally, I had to work on that for MariaDB 10.2 support, see #2825. I took the same approach as you did using quoteStringLiteral(). I added functional tests and that's where the things became more complex :(

Note that for MariaDB, I faced a strange issue when backslashing 'single-quotes'... While ' (backslash + single quote) and '' (double single-quotes) should be equivalent (see https://dev.mysql.com/doc/refman/5.7/en/string-literals.html). MariaDB 10.2 silently 'normalize' them when they are stored in information_schema (' -> '').

So for now #2825 handles backslashes and double-single-quotes and prevent schema diff (important) but keeps limited to MySQL/MariaDB families.

I will continue to work on this, but we might share some thoughts if you like to.

PS: See #2824 changed files and search for 'literal'

belgattitude · 2017-09-27T15:28:53Z

@PowerKiKi,

Thanks for your work, I would love to see automatic quoting of default values ;)

I was just wondering what happen in this scenario:

If some users have manually quoted their default values:
* @ORM\Column(type="string", options={"default" = "O\'Connor"})
When doing a schema update, the following change will happen:
ALTER TABLE x CHANGE t t VARCHAR(255) DEFAULT 'O\\''Connor' NOT NULL
This is a BC-BREAK, and users needs to update their models like:
* @ORM\Column(type="string", options={"default" = "O'Connor"})

Am I wrong ? or do you have any thought on this ?

morozov · 2017-09-27T16:07:40Z

@belgattitude please see some thoughts on BC at the end of #2850 (comment). While this behavior is indeed being changed, I can't see it documented anywhere. Did you see any documentation article saying that the value should be escaped?

belgattitude · 2017-09-27T16:56:25Z

@morozov, no it's not stated in the doc.

But I would be careful with this... I checked some usage of default through github code search:

https://github.com/search?p=1&q=%40ORM%5CColumn%28type%3D%22string%22+options+%22default%22%3A++%5C%5C+language%3APHP+extension%3Aphp&type=Code&utf8=%E2%9C%93

And haven't found any example of people manually backslashing default values yet. But I can imagine some did with things like 'Framework\\Application\' or even '\'FUNCTION()\''...

I'm really in favor of escaping default values, but I think it could be nice to add notes in UPGRADE and maybe give some tips:

A regexp to identify manually quoted defaults
A note to use 'columnDefinition' if a project needs to support pre- and *post-*auto-escaping doctrine versions).

But for me, all is good. I was just wondering for myself.

belgattitude · 2017-09-27T17:00:11Z

@morozov , please excuse my previous answer. I'll get some rest first ;) and check more deeply later... most of my concerns might have been solved by previous @PowerKiKi answers. Thanks

morozov · 2017-09-28T03:06:49Z

@belgattitude your concerns are perfectly valid. This change will require changes in the depending code if the default values there require escaping and are escaped.

Despite the fact that these values may exist (not only in the form of @ORM annotations and not only in public projects), it's unclear to me if a change in non-documented behavior can be considered BC-breaking. Someone more experienced than I am could probably clarify that.

I'm really in favor of escaping default values, but I think it could be nice to add notes in UPGRADE and maybe give some tips.

It's not really a question of personal preference, it's the only valid solution to this problem. Of course, upgrade notes will help.

A regexp to identify manually quoted defaults

It may help but it will work only in the case of using DBAL with ORM. Instead, without any additional tools, the user can upgrading and seeing if any unexpected schema changes are generated. It would be a bulletproof solution which doesn't require any knowledge about which characters to look for on our end. Who upgrades dependencies without testing?

A note to use 'columnDefinition' if a project needs to support pre- and *post-*auto-escaping doctrine versions).

Do you have an example of a project which needs to support multiple DBAL/ORM versions? I have a feeling that most of the packages which depend on DBAL/ORM are applications (not libraries), and thus they lock their dependencies by committing composer.lock.

I wouldn't recommend this approach even as a workaround because the value escaped in the definition is not portable.

PowerKiKi · 2017-09-28T04:51:27Z

@belgattitude, I was following your PR for a while and was wondering when our PRs would clash :-)

I believe morozov answered all your questions already. But yes, I think this PR will introduce a "minor" BC break (as called in UPGRADE.md).

@morozov, I will eventually resume work on this PR, probably in a few days. But right now I need to focus on other priorities.

belgattitude · 2017-09-28T09:33:28Z

@PowerKiKi, clash but very helpful to me

I finally read your description with a clear mind :) and just tested the backslash issue in default. I can confirm the endless schema diff (at least from annotations, don't know for yaml and xml, but let's assume they work the same).

So to me this change looks is more a bugfix (user perpective: hey doctrine handle my backslashes in defaults without triggering diff, thanks !) and I guess people naturally avoided backslash for this reason. One case still need attention, escape by repeating single quotes (i.e. "O''Connor"). Was working perfect but while possible, I doubt there was a lot of use anyway. On the other hand, we don't break any documented feature afaik.

So more 'bugfix' than 'bc'. Green for me.

@morozov

Do you have an example of a project which needs to support multiple DBAL/ORM versions?

I guess owncloud, nextcloud, sylius... applications/plugins could potentially be affected, I've already partially checked from source if those projects were quoting default values. Nothing yet, but I'll keep you informed next week, I'll install a bunch of them and check information_schema.

belgattitude · 2017-10-11T10:03:46Z

FYI, just tested nextcloud 12 to see if they set default with escape chars...
(SELECT * FROM INFORMATION_SCHEMA.COLUMNS WHERE TABLE_SCHEMA='nextcloud') and nothing that will break with your P/R ;)

belgattitude · 2017-10-14T12:41:17Z

@PowerKiKi Belgium is sunny today, I'm going to enjoy it ! I see you're working today, so sorry for you ;)

Two things that might help you in your weekend quest:

Alternative notations (standards)

According to https://dev.mysql.com/doc/refman/5.7/en/string-literals.html#character-escape-sequences and/or https://mariadb.com/kb/en/library/string-literals/.

Another way to escape the quoting character is repeating it twice:

Brings the question about other platforms too... Haven't digged into it. But since MariaDB 10.2.7, whatever the notation you use (doubling ') or (')... The information schema will always store it in the (doubling ') and I fear they might have done it for standardization reasons. This can lead to broken tests with MariaDB > 10.2.7.

One thing you can do is to add the (doubling ') test, don't care about MariaDB 10.2.7 and when ready I'll normalize on my side (it's already the case: https://github.com/belgattitude/dbal/blob/ad52f401c8d543060eda564f85b772c10dba9cad/lib/Doctrine/DBAL/Schema/MySqlSchemaManager.php#L242)

SQLServerPlatforms:

Copy paste (temporarily) the method below the AbstractPlatformTestCase.php and run the tests. Haven't digged into, but helped me get a glimpse of what I'll face after covering mysql, postgres and oracle:

    public function testGetDefaultValueDeclarationSQLEscaped()
    {
        $field = [
            'type' => Type::getType('string'),
            'default' => "'O'Connor said: \"Hello\" \ \r'"
        ];

        self::assertSame(sprintf(
                ' DEFAULT %s',
                $this->_platform->quoteStringLiteral("'O'Connor said: \"Hello\" \ \r'")
            ),
            // As demonstration: should be replaced by real expectation behaviour
            // in all platform. Made to illustrate problem with SQLServer
            $this->_platform->getDefaultValueDeclarationSQL($field)
        );
    }

Hope it helps and thanks a lot !!!!

belgattitude · 2017-10-14T12:44:41Z

@PowerKiKi FYI I might be unavailable till tuesday afternoon. But don't hesitate to ask details... I just gave you more problems than solutions ;)

PowerKiKi · 2017-10-15T03:01:51Z

@morozov I re-added the functional test for all platforms, and added support for PostgreSQL and SQLite.

continuousphp is failing, but I don't really understand what is is supposed to do or why it is failing...

Anyway, could you please have a look again at the code ?

belgattitude · 2017-10-15T09:15:52Z

@PowerKiKi Ì could not resist to open my laptop

For the SQLServer issue, you may have luck replacing return " DEFAULT '" . $field['default'] . "'"; by return " DEFAULT ".$this->quoteStringLiteral($field['default']); in SQLServerPlatform::getDefaultValueDeclarationSQL($field)

Then adding the test in AbstractSQLServerTestCase.

Never used sql server, but this could help:

https://docs.microsoft.com/en-us/sql/integration-services/expressions/numeric-string-and-boolean-literals, look at the string literal section.
https://stackoverflow.com/questions/1586560/how-do-i-escape-a-single-quote-in-sql-server for <doubling '>

Hope it helps.

belgattitude · 2017-10-15T09:27:44Z

@PowerKiKi

AFAIK, single quote escape by writing two adjacent double quotes is supported on SQL Server, Postgres, Maria, Oracle and Mysql.

Could you add tests for it ? (ie. $default="{ message: ''quoted by adjacent single quotes''}" or similar)

PowerKiKi · 2017-10-15T13:26:26Z

@belgattitude I am not willing to install SQLServer to work on that. This will be too much work for me for a platform that I don't really care about. Especially since Travis tests are passing. If SQLServer support is required, then somebody else will have to do it.

I can however include a double single quote in the tested value, to be sure that it works. So, it's done.

Ocramius · 2018-01-02T07:18:22Z

Still one CS issue detected, sorry @PowerKiKi

PowerKiKi · 2018-01-02T08:05:34Z

No worries, I should have checked more carefully.

BTW is there any reason why you don't use phpcbf to fix the code style of existing code once and for all ? or PHP-CS-Fixer for that matter ? I feel it would make it easier to check for code style locally (without the need of git-phpcs) and thus reduce the time spent (yours and mine) on those silly issues.

PhpSpreadsheet use composer scripts to easily check and fix code style. I found it very convenient to use.

lcobucci · 2018-01-02T08:15:29Z

@PowerKiKi phpcs and phpcbf are properly configured but we have many violations that need to be fixed and we decided to fix them in the develop branch, in order to reduce the amount of conflicts in the existing contributions.

Ocramius · 2018-01-02T08:40:20Z

🚢 thanks for the patience here, @PowerKiKi!

PowerKiKi · 2018-01-02T08:41:53Z

Thanks for the merge, and happy new year 🎉

Ocramius · 2018-01-02T08:48:46Z

Seems like one failure slipped through on oci8: https://continuousphp.com/git-hub/doctrine/dbal/build/46f18bd9-51d3-4d68-a132-fea0b189058f/output

PowerKiKi · 2018-01-02T08:58:14Z

It seems to be related to Oracle, and I am really not familiar with that system. I am afraid I won't be able to help with that.

Ocramius · 2018-01-02T09:32:30Z

That's alright, but we may need to revert the merge until we found the culprit.

Majkl578 · 2018-01-02T14:22:00Z

I see this was merged into master, shouldn't it go only to develop since it's BC break?
Also wondering if this could somehow be related to #2926.

Ocramius · 2018-01-02T14:29:47Z

@Majkl578 no, 2.7 is fine, as this can really only cause some issues with the schema tool (and those happen at every upgrade)

morozov · 2018-01-03T07:25:39Z

@PowerKiKi please take a look at the failures on Oracle:

<testcase name="testEscapedDefaultValueCanBeIntrospected" class="Doctrine\Tests\DBAL\Functional\Schema\OracleSchemaManagerTest" classname="Doctrine.Tests.DBAL.Functional.Schema.OracleSchemaManagerTest" file="/home/cphp/data/container/9d05fb86-47cc-453a-ac6a-4168888350f7/workspace/tests/Doctrine/Tests/DBAL/Functional/Schema/OracleSchemaManagerTest.php" line="1443" assertions="1" time="0.883744">
    <failure type="PHPUnit\Framework\ExpectationFailedException">
Doctrine\Tests\DBAL\Functional\Schema\OracleSchemaManagerTest::testEscapedDefaultValueCanBeIntrospected
  | should be able introspect the value of default for: An ASCII NUL (X'00')
  | Failed asserting that two strings are identical.
  | --- Expected
  | +++ Actual
  | @@ @@
  | -'foo\0bar'
  | +'foo\\0bar'
  |  
  | /home/cphp/data/container/9d05fb86-47cc-453a-ac6a-4168888350f7/workspace/tests/Doctrine/Tests/DBAL/Functional/Schema/SchemaManagerFunctionalTestCase.php:1449
    </failure>
</testcase>
<testcase name="testEscapedDefaultValueCanBeInserted" class="Doctrine\Tests\DBAL\Functional\Schema\OracleSchemaManagerTest" classname="Doctrine.Tests.DBAL.Functional.Schema.OracleSchemaManagerTest" file="/home/cphp/data/container/9d05fb86-47cc-453a-ac6a-4168888350f7/workspace/tests/Doctrine/Tests/DBAL/Functional/Schema/OracleSchemaManagerTest.php" line="1453" assertions="0" time="0.586455">
    <error type="Exception">
Exception: [PHPUnit\Framework\Error\Notice] Undefined index: field0
  |  
  | With queries:
  | 6. SQL: 'SELECT * FROM sys.user_tables' Params:
  | 5. SQL: 'SELECT * FROM string_escaped_default_value' Params:
  | 4. SQL: 'INSERT INTO string_escaped_default_value (def_foo) VALUES (?)' Params: 'foo'
  | 3. SQL: 'CREATE TABLE string_escaped_default_value (field0 VARCHAR2(255) DEFAULT 'foo\\0bar' NOT NULL, field1 VARCHAR2(255) DEFAULT 'foo\\''bar' NOT NULL, field2 VARCHAR2(255) DEFAULT 'foo''''bar' NOT NULL, field3 VARCHAR2(255) DEFAULT 'foo\\"bar' NOT NULL, field4 VARCHAR2(255) DEFAULT 'foo""bar' NOT NULL, field5 VARCHAR2(255) DEFAULT 'foo\\bbar' NOT NULL, field6 VARCHAR2(255) DEFAULT 'foo\\nbar' NOT NULL, field7 VARCHAR2(255) DEFAULT 'foo\\rbar' NOT NULL, field8 VARCHAR2(255) DEFAULT 'foo\\tbar' NOT NULL, field9 VARCHAR2(255) DEFAULT 'foo\\Zbar' NOT NULL, field10 VARCHAR2(255) DEFAULT 'foo\\\\bar' NOT NULL, field11 VARCHAR2(255) DEFAULT 'foo\\%bar' NOT NULL, field12 VARCHAR2(255) DEFAULT 'foo\\_bar' NOT NULL, def_foo VARCHAR2(255) NOT NULL)' Params:
  | 2. SQL: 'DROP TABLE string_escaped_default_value' Params:
  | 1. SQL: 'DROP TRIGGER STRING_ESCAPED_DEFAULT_VALUE_AI_PK' Params:
  |  
  | Trace:
  | /home/cphp/data/container/9d05fb86-47cc-453a-ac6a-4168888350f7/workspace/vendor/symfony/phpunit-bridge/DeprecationErrorHandler.php:105
  | /home/cphp/data/container/9d05fb86-47cc-453a-ac6a-4168888350f7/workspace/tests/Doctrine/Tests/DBAL/Functional/Schema/SchemaManagerFunctionalTestCase.php:1460
  | /home/cphp/data/container/9d05fb86-47cc-453a-ac6a-4168888350f7/workspace/vendor/phpunit/phpunit/src/Framework/TestCase.php:1071
  | /home/cphp/data/container/9d05fb86-47cc-453a-ac6a-4168888350f7/workspace/vendor/phpunit/phpunit/src/Framework/TestCase.php:939
  | /home/cphp/data/container/9d05fb86-47cc-453a-ac6a-4168888350f7/workspace/vendor/phpunit/phpunit/src/Framework/TestResult.php:698
  | /home/cphp/data/container/9d05fb86-47cc-453a-ac6a-4168888350f7/workspace/vendor/phpunit/phpunit/src/Framework/TestCase.php:894
  | /home/cphp/data/container/9d05fb86-47cc-453a-ac6a-4168888350f7/workspace/vendor/phpunit/phpunit/src/Framework/TestSuite.php:755
  | /home/cphp/data/container/9d05fb86-47cc-453a-ac6a-4168888350f7/workspace/vendor/phpunit/phpunit/src/Framework/TestSuite.php:755
  | /home/cphp/data/container/9d05fb86-47cc-453a-ac6a-4168888350f7/workspace/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:546
  | /home/cphp/data/container/9d05fb86-47cc-453a-ac6a-4168888350f7/workspace/vendor/phpunit/phpunit/src/TextUI/Command.php:195
  | /home/cphp/data/container/9d05fb86-47cc-453a-ac6a-4168888350f7/workspace/vendor/phpunit/phpunit/src/TextUI/Command.php:148
  | /home/cphp/data/container/9d05fb86-47cc-453a-ac6a-4168888350f7/workspace/vendor/phpunit/phpunit/phpunit:53
  |  
  |  
  | /home/cphp/data/container/9d05fb86-47cc-453a-ac6a-4168888350f7/workspace/tests/Doctrine/Tests/DbalFunctionalTestCase.php:88
    </error>
</testcase>

Sorry, didn't notice the conversation above.

morozov · 2018-01-03T21:52:06Z

Looks like it's not just Oracle failures. The changes haven't been tested properly.

The test fixtures do not contain lower ASCII range characters as they claim. For instance,

dbal/tests/Doctrine/Tests/DBAL/Functional/Schema/SchemaManagerFunctionalTestCase.php

Line 1413 in e3ab127

['An ASCII NUL (X\'00\')', "foo\\0bar"],

creates a \ followed by a 0 character, not a NULL byte (\0). Same is for some other characters. Fixing these fixtures introduces failures on SQLite where the NULL-byte needs some additional escaping. By the way, the fact that the CREATE TABLE creates all fields at a time and fails once for all fields makes it harder to understand which exactly field it's failed because of.

Fixing failures on Oracle requires implementing more logic in the Oracle's schema manager and reverting some code introduced by Escape identifiers in metadata SQL properly when used as string literal #2442. Here, we have a disagreement between tests. The unit ones claim backslashes should be escaped while the functional ones (as well as manual testing) show they shouldn't. Additional research is needed.

The work in progress is tracked in #2960.

Ocramius · 2018-01-04T07:33:18Z

@morozov should I revert here or just wait?

morozov · 2018-01-04T07:50:21Z

Let’s revert it. Not sure if I’m going to be able to spend enough time on this shortly. And the failures are annoying.

…ackslashes" This reverts commit e3ab127, reversing changes made to f988b0a.

Ocramius · 2018-01-04T08:49:01Z

Reverted in a5d3e33 for now

…lt-values-with-backslashes"" This reverts commit a5d3e33.

…s-with-backslashes

PowerKiKi mentioned this pull request Sep 9, 2017

[BUG] Database analyzer Ecodev/newsletter#141

Closed

morozov reviewed Sep 20, 2017

View reviewed changes

morozov added Bug MySQL Schema Management labels Sep 20, 2017

morozov requested changes Sep 20, 2017

View reviewed changes

PowerKiKi force-pushed the default-values-with-backslashes branch from 4dfc5c2 to 44e1e17 Compare September 22, 2017 04:53

belgattitude mentioned this pull request Sep 27, 2017

MariaDB 10.2 initial support #2825

Merged

8 tasks

Fix code style

fa9e2b1

Ocramius added this to the 2.7.0 milestone Jan 2, 2018

Ocramius assigned Ocramius and unassigned PowerKiKi Jan 2, 2018

Fix code style (bis)

2971f86

Ocramius merged commit e3ab127 into doctrine:master Jan 2, 2018

Ocramius added a commit that referenced this pull request Jan 4, 2018

Revert "Merge pull request #2850 from PowerKiKi/default-values-with-b…

a5d3e33

…ackslashes" This reverts commit e3ab127, reversing changes made to f988b0a.

morozov mentioned this pull request Mar 14, 2018

Handle default values as values, not SQL expressions #2960

Merged

morozov removed this from the 2.7.0 milestone Apr 4, 2018

morozov mentioned this pull request Jun 4, 2018

Default value when Doctrine\DBAL\Schema\AbstractSchemaManager::listTableColumns #3172

Closed

morozov added a commit to morozov/dbal that referenced this pull request Apr 10, 2019

Revert "Revert "Merge pull request doctrine#2850 from PowerKiKi/defau…

39bc4df

…lt-values-with-backslashes"" This reverts commit a5d3e33.

morozov added a commit to morozov/dbal that referenced this pull request Apr 11, 2019

Revert "Revert "Merge pull request doctrine#2850 from PowerKiKi/defau…

73a2909

…lt-values-with-backslashes"" This reverts commit a5d3e33.

morozov added a commit to morozov/dbal that referenced this pull request Apr 11, 2019

Restored relevant changes from doctrine#2850, PowerKiKi/default-value…

dd45ef5

…s-with-backslashes

morozov added a commit to morozov/dbal that referenced this pull request Apr 18, 2019

Restored relevant changes from doctrine#2850, PowerKiKi/default-value…

8515d7f

…s-with-backslashes

github-actions bot locked as resolved and limited conversation to collaborators Aug 12, 2022

Support for backslashes in default values #2850

Support for backslashes in default values #2850

Conversation

PowerKiKi commented Sep 9, 2017

greg0ire commented Sep 19, 2017

morozov Sep 20, 2017

Choose a reason for hiding this comment

PowerKiKi Sep 20, 2017 • edited Loading

Choose a reason for hiding this comment

morozov Sep 20, 2017

Choose a reason for hiding this comment

morozov Sep 20, 2017 • edited Loading

Choose a reason for hiding this comment

PowerKiKi Sep 20, 2017

Choose a reason for hiding this comment

morozov Sep 20, 2017 • edited Loading

Choose a reason for hiding this comment

PowerKiKi Sep 20, 2017

Choose a reason for hiding this comment

morozov Sep 20, 2017

Choose a reason for hiding this comment

morozov left a comment

Choose a reason for hiding this comment

PowerKiKi commented Sep 21, 2017

PowerKiKi commented Sep 21, 2017

morozov commented Sep 21, 2017

PowerKiKi commented Sep 22, 2017 • edited Loading

morozov commented Sep 22, 2017 • edited Loading

belgattitude commented Sep 27, 2017

belgattitude commented Sep 27, 2017

morozov commented Sep 27, 2017

belgattitude commented Sep 27, 2017 • edited Loading

belgattitude commented Sep 27, 2017

morozov commented Sep 28, 2017 • edited Loading

PowerKiKi commented Sep 28, 2017

belgattitude commented Sep 28, 2017

belgattitude commented Oct 11, 2017

belgattitude commented Oct 14, 2017

belgattitude commented Oct 14, 2017

PowerKiKi commented Oct 15, 2017

belgattitude commented Oct 15, 2017

belgattitude commented Oct 15, 2017 • edited Loading

PowerKiKi commented Oct 15, 2017

Ocramius commented Jan 2, 2018

PowerKiKi commented Jan 2, 2018

lcobucci commented Jan 2, 2018 • edited Loading

Ocramius commented Jan 2, 2018

PowerKiKi commented Jan 2, 2018

Ocramius commented Jan 2, 2018

PowerKiKi commented Jan 2, 2018

Ocramius commented Jan 2, 2018

Majkl578 commented Jan 2, 2018

Ocramius commented Jan 2, 2018

morozov commented Jan 3, 2018 • edited Loading

morozov commented Jan 3, 2018 • edited Loading

Ocramius commented Jan 4, 2018

morozov commented Jan 4, 2018 • edited Loading

Ocramius commented Jan 4, 2018

PowerKiKi Sep 20, 2017 •

edited

Loading

morozov Sep 20, 2017 •

edited

Loading

morozov Sep 20, 2017 •

edited

Loading

PowerKiKi commented Sep 22, 2017 •

edited

Loading

morozov commented Sep 22, 2017 •

edited

Loading

belgattitude commented Sep 27, 2017 •

edited

Loading

morozov commented Sep 28, 2017 •

edited

Loading

belgattitude commented Oct 15, 2017 •

edited

Loading

lcobucci commented Jan 2, 2018 •

edited

Loading

morozov commented Jan 3, 2018 •

edited

Loading

morozov commented Jan 3, 2018 •

edited

Loading

morozov commented Jan 4, 2018 •

edited

Loading