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

[DDC-2825] Fix persistence on a table with a schema on a platform without schema support #881

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
10 participants
@michaelperrin
Contributor

michaelperrin commented Dec 18, 2013

This PR solves two related issues with the use of a database schema on platforms (such as SQLite) that don't support schemas.

I discovered the issues when I generated the schema from my Doctrine entities on SQLite (for unit test purposes of my application) whereas my main application uses PostgreSQL.

This is one of my first PR on Doctrine, so sorry if I made some things in the wrong way and I'm open to discussion.

First problem: table names dots are not converted in the ORM

On a platform like SQLite, DBAL converts table names with dots (ie. a schema is declared) to double underscores.
However, the ORM doesn't do it, and persisting leads to an exception.

Example:

MyNamespace\Mytable:
    type: entity
    table: myschema.mytable
    # ...

And then somewhere in the code:

$myTable = new MyNamespace\Mytable();
$entityManager->persist($myTable);
$entityManager->flush();

This doesn't work in the current version of Doctrine. The table is created as myschema__mytable but entities are unsuccessfully saved in myschema.mytable.

Second problem: table names with reserved keywords in a database schema are not correctly escaped

When a table name is declared as myschema.order (or any other reserved keyword), only the reserved keyword part is escaped when creating the table, leading to the creation of a table name like myschema__order, which is invalid and therefore fails.

How this PR solves the problem

The classmetadata now stores in 2 separated properties the name of the table and the name of the schema. The schema property was partially implemented but I now make a full use of it.

When metadata is read (from Annotations, YAML, ...), if the table name has a dot (myschema.mytable), it's splitted into 2 parts, and myschema is saved in the schema table property, and mytable is saved in the name table property, instead of storing the whole myschema.mytable in the name table property.

This allows to do specific things about schemas everywhere in Doctrine, and not splitting again parts everywhere it's needed.

By the way, the schema property can now fully be used.

For instance, these 2 YAML configurations are valid and do the same thing:

MyNamespace\Mytable:
    type: entity
    table: myschema.mytable

and:

MyNamespace\Mytable:
    type: entity
    table: mytable
    schema: myschema

This was something which was not finished to be implented since Doctrine 2.0.

The Default quote strategy now converts back the schema and table names to a unique table name, depending on the platform (e.g. myschema.mytable if the platform supports schemas, and myschema__mytable otherwise).

As a result, there is no problem anymore and entities can be persisted without getting any exception.
I added some unit tests for this (the same unit tests failed before of course).

There's however a slight tradeoff on performance, as the getTableName of the DefaultQuoteStrategy class adds some tests to return the correct table name.

This solved these Doctrine issues: DDC-2825 and DDC-2636.

Again, this is one of my first PRs on Doctrine, so if there's anything wrong or if you have any question, feel free to comment this PR.

@Ocramius This PR is about what we talked about in DDC-2825

@doctrinebot

This comment has been minimized.

Show comment
Hide comment
@doctrinebot

doctrinebot Dec 18, 2013

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DDC-2861

We use Jira to track the state of pull requests and the versions they got
included in.

doctrinebot commented Dec 18, 2013

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DDC-2861

We use Jira to track the state of pull requests and the versions they got
included in.

@michaelperrin

This comment has been minimized.

Show comment
Hide comment
@michaelperrin

michaelperrin Dec 18, 2013

Contributor

I didn't notice that a unit test now fails with this PR (the one concerning DBAL-204 on MySQL). I'm going to have a look at it.

Contributor

michaelperrin commented Dec 18, 2013

I didn't notice that a unit test now fails with this PR (the one concerning DBAL-204 on MySQL). I'm going to have a look at it.

@michaelperrin

This comment has been minimized.

Show comment
Hide comment
@michaelperrin

michaelperrin Dec 18, 2013

Contributor

All unit tests for all platforms are now fixed.

Contributor

michaelperrin commented Dec 18, 2013

All unit tests for all platforms are now fixed.

Show outdated Hide outdated lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php
$sequenceName = $class->getTableName() . '_' . $columnName . '_seq';
$definition = array(
if ($schemaName = $class->getSchemaName()) {

This comment has been minimized.

@beberlei

beberlei Jan 2, 2014

Member

can you extract that block and the one above into a private helper method? also don't use else then, but make it an early return for the matching if only.

@beberlei

beberlei Jan 2, 2014

Member

can you extract that block and the one above into a private helper method? also don't use else then, but make it an early return for the matching if only.

This comment has been minimized.

@deeky666

deeky666 Jan 3, 2014

Member

@beberlei That is not necessary. This block is already changed by PR #890 and should be covered by DBAL. Also see comment above.

@deeky666

deeky666 Jan 3, 2014

Member

@beberlei That is not necessary. This block is already changed by PR #890 and should be covered by DBAL. Also see comment above.

Show outdated Hide outdated lib/Doctrine/ORM/Mapping/Driver/AnnotationDriver.php
$schemaName = $tableAnnot->schema;
// Split schema and table name from a table name like "myschema.mytable"
if ( ! empty($tableName) && strpos($tableName, '.')) {

This comment has been minimized.

@beberlei

beberlei Jan 2, 2014

Member

Change to just if (strpos($tableName, '.') !== false) { please

@beberlei

beberlei Jan 2, 2014

Member

Change to just if (strpos($tableName, '.') !== false) { please

Show outdated Hide outdated lib/Doctrine/ORM/Mapping/Driver/XmlDriver.php
$tableName = (string)$xmlRoot['table'];
// Split schema and table name from a table name like "myschema.mytable"
if (strpos($tableName, '.')) {

This comment has been minimized.

@beberlei

beberlei Jan 2, 2014

Member

add !== false please

@beberlei

beberlei Jan 2, 2014

Member

add !== false please

Show outdated Hide outdated lib/Doctrine/ORM/Mapping/Driver/YamlDriver.php
$tableName = $element['table'];
// Split schema and table name from a table name like "myschema.mytable"
if (strpos($tableName, '.')) {

This comment has been minimized.

@beberlei

beberlei Jan 2, 2014

Member

add !== false please

@beberlei

beberlei Jan 2, 2014

Member

add !== false please

@beberlei

This comment has been minimized.

Show comment
Hide comment
@beberlei

beberlei Jan 2, 2014

Member

Very good PR, please fix the issues mentioned, squash all commits and rebase to master.

Member

beberlei commented Jan 2, 2014

Very good PR, please fix the issues mentioned, squash all commits and rebase to master.

Show outdated Hide outdated lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php
$sequenceName = $class->getTableName() . '_' . $columnName . '_seq';
}
$definition = array(
'sequenceName' => $this->targetPlatform->fixSchemaElementName($sequenceName)

This comment has been minimized.

@deeky666

deeky666 Jan 3, 2014

Member

@beberlei Btw this line looks dangerous to me because it changes and trims the sequence name on the fly if necessary and does not guarantee it to match anymore with the sequence in the database created by DBAL. IMO ORM should not do something like that. It should be coming valid from DBAL in the first place.
The main concern here is about Oracle where sequence names cannot be longer than 30 charachters. It will silently break emulated autoincrementation via triggers as the trigger references the sequence by name which will fail if it was renamed here and return 0 again when calling lastInsertId().
We should think about creating a unique name like for unnamed indexes, unique and foreign key constraints in DBAL to come around this limitation. Please also see PR #890. But I don't know if that would break BC.

@deeky666

deeky666 Jan 3, 2014

Member

@beberlei Btw this line looks dangerous to me because it changes and trims the sequence name on the fly if necessary and does not guarantee it to match anymore with the sequence in the database created by DBAL. IMO ORM should not do something like that. It should be coming valid from DBAL in the first place.
The main concern here is about Oracle where sequence names cannot be longer than 30 charachters. It will silently break emulated autoincrementation via triggers as the trigger references the sequence by name which will fail if it was renamed here and return 0 again when calling lastInsertId().
We should think about creating a unique name like for unnamed indexes, unique and foreign key constraints in DBAL to come around this limitation. Please also see PR #890. But I don't know if that would break BC.

@michaelperrin

This comment has been minimized.

Show comment
Hide comment
@michaelperrin

michaelperrin Jan 3, 2014

Contributor

Thank you @beberlei for your comments, this is now fixed!

As @deeky666 pointed out, I had to make some further changes when rebasing, as some changes have recently been made on PR #890 .

I had to use a little workaround to avoid break compatibility issues on PostgreSQL, so that sequence names have the same before and after this PR changes.

As there is now a DBAL method to get the sequence name, based on table name and column name (see doctrine/dbal#428), I have to prepend the schema name to the table name before delegating the sequence name to DBAL. Maybe there should be a third optional argument for the schema name on the getIdentitySequenceName method on AbstractPlatform.php in DBAL.

Everything works fine, apart from the current build problem on the master branch.

If you have any other comment to improve the code, I'll be glad to help.

Contributor

michaelperrin commented Jan 3, 2014

Thank you @beberlei for your comments, this is now fixed!

As @deeky666 pointed out, I had to make some further changes when rebasing, as some changes have recently been made on PR #890 .

I had to use a little workaround to avoid break compatibility issues on PostgreSQL, so that sequence names have the same before and after this PR changes.

As there is now a DBAL method to get the sequence name, based on table name and column name (see doctrine/dbal#428), I have to prepend the schema name to the table name before delegating the sequence name to DBAL. Maybe there should be a third optional argument for the schema name on the getIdentitySequenceName method on AbstractPlatform.php in DBAL.

Everything works fine, apart from the current build problem on the master branch.

If you have any other comment to improve the code, I'll be glad to help.

@deeky666

This comment has been minimized.

Show comment
Hide comment
@deeky666

deeky666 Jan 3, 2014

Member

@michaelperrin @beberlei While I definitely like this PR I'm afraid this is a BC break because it will break receiving lastInderstId for existing tables. Tables already created with a sequence in the format <table>_<column>_seq will not be compatible with the new <schema>_<table>_<column>_seq format.
If we should decide that this break is acceptable, I would suggest some further improvements:

  1. Pass the full qualified table name <schema>.<table> to AbstractPlatform::getIdentitySequenceName() and generate a unique hash key for the sequence name like SEQ_<HASH> (as already done for other asset names, see previous comment) to avoid problems with identifier length in Oracle and other platforms.
  2. AbstractPlatform::getIdentitySequenceName() needs to check and split the full qualified table name into schema and table like done in other DBAL methods already.

Otherwise this PR would only fix one problem and probably introduce others for max identifier length and sequence name matching of existing tables.

Member

deeky666 commented Jan 3, 2014

@michaelperrin @beberlei While I definitely like this PR I'm afraid this is a BC break because it will break receiving lastInderstId for existing tables. Tables already created with a sequence in the format <table>_<column>_seq will not be compatible with the new <schema>_<table>_<column>_seq format.
If we should decide that this break is acceptable, I would suggest some further improvements:

  1. Pass the full qualified table name <schema>.<table> to AbstractPlatform::getIdentitySequenceName() and generate a unique hash key for the sequence name like SEQ_<HASH> (as already done for other asset names, see previous comment) to avoid problems with identifier length in Oracle and other platforms.
  2. AbstractPlatform::getIdentitySequenceName() needs to check and split the full qualified table name into schema and table like done in other DBAL methods already.

Otherwise this PR would only fix one problem and probably introduce others for max identifier length and sequence name matching of existing tables.

@michaelperrin

This comment has been minimized.

Show comment
Hide comment
@michaelperrin

michaelperrin Jan 3, 2014

Contributor

@deeky666 Thank you for your comments. Tell me if I am wrong, but schema name was already included in sequence names before this PR.

As schemas were included as to define a schema, the table name had to be defined as schemaname.tablename, (which is still possible after this PR). The difference is that the getTableName of the ClassMetadataInfo class now only returns tablename instead of schemaname.tablename.

Still, you are right as there is a difference: the sequence name was schemaname.tablename_columnname_seq and it is now schemaname_tablename_columnname_seq (a dot replaced by an underscore).

I like your second idea: AbstractPlatform::getIdentitySequenceName() could be implemented to replace the dot by an underscore (or a hash) for platforms that don't support schemas, and keep the dot for platforms that support schemas.

We keep this way full BC, and the ClassMetadataFactory keeps the same as before this PR.

What do you think? If you think that's the way to go, I update this PR and open a PR on DBAL.

Contributor

michaelperrin commented Jan 3, 2014

@deeky666 Thank you for your comments. Tell me if I am wrong, but schema name was already included in sequence names before this PR.

As schemas were included as to define a schema, the table name had to be defined as schemaname.tablename, (which is still possible after this PR). The difference is that the getTableName of the ClassMetadataInfo class now only returns tablename instead of schemaname.tablename.

Still, you are right as there is a difference: the sequence name was schemaname.tablename_columnname_seq and it is now schemaname_tablename_columnname_seq (a dot replaced by an underscore).

I like your second idea: AbstractPlatform::getIdentitySequenceName() could be implemented to replace the dot by an underscore (or a hash) for platforms that don't support schemas, and keep the dot for platforms that support schemas.

We keep this way full BC, and the ClassMetadataFactory keeps the same as before this PR.

What do you think? If you think that's the way to go, I update this PR and open a PR on DBAL.

@deeky666

This comment has been minimized.

Show comment
Hide comment
@deeky666

deeky666 Jan 3, 2014

Member

@michaelperrin Hmm I'm not getting entirely through this but you might be right. But what seems wrong is how the sequence name is now passed to the platform if the table name contans a schema prefix because it now contains an underscore instead of a dot. On PostgreSQL for example it was schema.table before and now is schema_table, right? This would be wrong then. Also I wonder if you should not evaluate AbstractPlatform::supportsSchemas for dotted to underscore notation conversion instead (not entirely sure).

Member

deeky666 commented Jan 3, 2014

@michaelperrin Hmm I'm not getting entirely through this but you might be right. But what seems wrong is how the sequence name is now passed to the platform if the table name contans a schema prefix because it now contains an underscore instead of a dot. On PostgreSQL for example it was schema.table before and now is schema_table, right? This would be wrong then. Also I wonder if you should not evaluate AbstractPlatform::supportsSchemas for dotted to underscore notation conversion instead (not entirely sure).

@michaelperrin

This comment has been minimized.

Show comment
Hide comment
@michaelperrin

michaelperrin Jan 3, 2014

Contributor

@deeky666 Yes, this is what I realized with your previous comment, there is a change on the PostgreSQL platform.
Do you think I should check for platform support in the ORM and decide in my ClassMetadataFactory::getSchemaName method whether to convert the dot to an underscore or not (depending on platform support for schemas), or should this be handled in DBAL? The first solution has the advantage to be easier, the second one might be a bit more flexible.

Contributor

michaelperrin commented Jan 3, 2014

@deeky666 Yes, this is what I realized with your previous comment, there is a change on the PostgreSQL platform.
Do you think I should check for platform support in the ORM and decide in my ClassMetadataFactory::getSchemaName method whether to convert the dot to an underscore or not (depending on platform support for schemas), or should this be handled in DBAL? The first solution has the advantage to be easier, the second one might be a bit more flexible.

@deeky666

This comment has been minimized.

Show comment
Hide comment
@deeky666

deeky666 Jan 3, 2014

Member

@michaelperrin I am not sure if understand the root problem correctly but I'll try to explain my understanding. The problem is that table names that contain a dot do not work on platforms that do not support schemas/schema notation. Then my understanding is that this is a portability fix because why would you define your table names with dots then if you know that the platform does not support that kind of notation? If that is the case I would fix that entirely in ORM as you would not expect platforms that do not support this kind of notation to check for that and fix it by themselves. Then AbstractPlatform::supportsSchemas is the way to check for whether to convert or not in this case IMO. But as I said I am not entirely sure about this as I am not too much into ORM. Maybe someone else can provide feedback on that, too.

Member

deeky666 commented Jan 3, 2014

@michaelperrin I am not sure if understand the root problem correctly but I'll try to explain my understanding. The problem is that table names that contain a dot do not work on platforms that do not support schemas/schema notation. Then my understanding is that this is a portability fix because why would you define your table names with dots then if you know that the platform does not support that kind of notation? If that is the case I would fix that entirely in ORM as you would not expect platforms that do not support this kind of notation to check for that and fix it by themselves. Then AbstractPlatform::supportsSchemas is the way to check for whether to convert or not in this case IMO. But as I said I am not entirely sure about this as I am not too much into ORM. Maybe someone else can provide feedback on that, too.

Show outdated Hide outdated mysql.phpunit.xml
@@ -0,0 +1,42 @@
<?xml version="1.0" encoding="utf-8"?>

This comment has been minimized.

@deeky666

deeky666 Jan 3, 2014

Member

Are you sure you wanted to commit this file into the PR?

@deeky666

deeky666 Jan 3, 2014

Member

Are you sure you wanted to commit this file into the PR?

This comment has been minimized.

@michaelperrin

michaelperrin Jan 3, 2014

Contributor

Aw, sorry, not really used to Github Mac software. Back to command line! This is fixed.

@michaelperrin

michaelperrin Jan 3, 2014

Contributor

Aw, sorry, not really used to Github Mac software. Back to command line! This is fixed.

This comment has been minimized.

@deeky666

deeky666 Jan 3, 2014

Member

No problem :)

@deeky666

deeky666 Jan 3, 2014

Member

No problem :)

@michaelperrin

This comment has been minimized.

Show comment
Hide comment
@michaelperrin

michaelperrin Jan 3, 2014

Contributor

@deeky666 Concerning your last comment, I'm going to try to make things a bit clearer.

The short answer: this PR aims to make ORM entities work on all platforms even when schemas are used, as the ORM is an abstraction layer.

Longer answer: Why is it useful to make schemas work on platforms that don't support them?

Here is my use case that made me discover the issue: my web project is developed on PostgreSQL and we make use of schemas (table names are therefore declared with dots).

I want to run unit tests of my project on a clean database. The idea is to create the schema structure in SQLite at the beginning of unit tests, run tests, and destroy the database. This currently doesn't work, as whereas DBAL is able to create the database structure (by emulating schemas), the ORM doesn't manage to read and save entities within this same database.

The ORM should be able to build structure and use entities for any database platform, based on the same database structure configuration (Annotations, YAML files, XML files, etc.). It works actually pretty fine, except this case.

This PR aim is twofold:

  • Fix conversion of dots for platforms that emulate schemas (as done in DBAL) so that entities work
  • Better separation of schema name and table name (instead of declaring {table: myschema.mytable}, it can be declared as {table: mytable, schema: myschema}). The first notation is still available, but things are clearly separated at execution time.

I hope it's a bit more clear.

Contributor

michaelperrin commented Jan 3, 2014

@deeky666 Concerning your last comment, I'm going to try to make things a bit clearer.

The short answer: this PR aims to make ORM entities work on all platforms even when schemas are used, as the ORM is an abstraction layer.

Longer answer: Why is it useful to make schemas work on platforms that don't support them?

Here is my use case that made me discover the issue: my web project is developed on PostgreSQL and we make use of schemas (table names are therefore declared with dots).

I want to run unit tests of my project on a clean database. The idea is to create the schema structure in SQLite at the beginning of unit tests, run tests, and destroy the database. This currently doesn't work, as whereas DBAL is able to create the database structure (by emulating schemas), the ORM doesn't manage to read and save entities within this same database.

The ORM should be able to build structure and use entities for any database platform, based on the same database structure configuration (Annotations, YAML files, XML files, etc.). It works actually pretty fine, except this case.

This PR aim is twofold:

  • Fix conversion of dots for platforms that emulate schemas (as done in DBAL) so that entities work
  • Better separation of schema name and table name (instead of declaring {table: myschema.mytable}, it can be declared as {table: mytable, schema: myschema}). The first notation is still available, but things are clearly separated at execution time.

I hope it's a bit more clear.

@deeky666

This comment has been minimized.

Show comment
Hide comment
@deeky666

deeky666 Jan 3, 2014

Member

@michaelperrin Thank you for that explanation. I think I get it now. So in my opinion you should use dotted notation if the underlying platform supports schemas, use double underscore for platforms that do not support schemas but can emulate. This leaves the question open about platforms that do neither support nor emulate schemas. Is the dotted notation acceptable here on all platforms? As for MySQL it works because it qualifies the database instead. But what about the others? Is that notation acceptable on all platforms then? Another question I have is about the double underscore notation. Is that a convention by Doctrine for all schema emulating platforms or is it SQLite specific?
I don't want to be too picky here. Just make up my mind about the implications of this. IMO the best solution would be to provide some methods on AbstractPlatform to let each platform decide how to deal with each case (but that is out of scope of this PR and might only be a nice extension at some later point).

Member

deeky666 commented Jan 3, 2014

@michaelperrin Thank you for that explanation. I think I get it now. So in my opinion you should use dotted notation if the underlying platform supports schemas, use double underscore for platforms that do not support schemas but can emulate. This leaves the question open about platforms that do neither support nor emulate schemas. Is the dotted notation acceptable here on all platforms? As for MySQL it works because it qualifies the database instead. But what about the others? Is that notation acceptable on all platforms then? Another question I have is about the double underscore notation. Is that a convention by Doctrine for all schema emulating platforms or is it SQLite specific?
I don't want to be too picky here. Just make up my mind about the implications of this. IMO the best solution would be to provide some methods on AbstractPlatform to let each platform decide how to deal with each case (but that is out of scope of this PR and might only be a nice extension at some later point).

@michaelperrin

This comment has been minimized.

Show comment
Hide comment
@michaelperrin

michaelperrin Jan 6, 2014

Contributor

@deeky666 I have fixed the name of sequences for platforms that support schemas. They are now named like myschema.mytable_mycolumn_seq as before, instead of myschema_mytable_mycolumn_seq as the previous version of this PR introduced.

SQLite is currently the only one platform emulating schemas and it uses double underscores (see the SqlitePlatform class in DBAL). A bit off the topic, but the use of PHP traits like "SchemaSupportPlatform" and "SchemaEmulatedPlatform" and so on would be a nice addition in future versions of DBAL (but that wouldn't work on PHP 5.3 anymore).

I agree with you. This PR can be a first step that solves the current problem and gives better support of schemas, and then we should enhance AbstractPlatform on DBAL in an other PR. I would be glad to help on this.

@beberlei What do you think about this PR? Do you think it can be merged and enhancements can be made later on on DBAL?

Contributor

michaelperrin commented Jan 6, 2014

@deeky666 I have fixed the name of sequences for platforms that support schemas. They are now named like myschema.mytable_mycolumn_seq as before, instead of myschema_mytable_mycolumn_seq as the previous version of this PR introduced.

SQLite is currently the only one platform emulating schemas and it uses double underscores (see the SqlitePlatform class in DBAL). A bit off the topic, but the use of PHP traits like "SchemaSupportPlatform" and "SchemaEmulatedPlatform" and so on would be a nice addition in future versions of DBAL (but that wouldn't work on PHP 5.3 anymore).

I agree with you. This PR can be a first step that solves the current problem and gives better support of schemas, and then we should enhance AbstractPlatform on DBAL in an other PR. I would be glad to help on this.

@beberlei What do you think about this PR? Do you think it can be merged and enhancements can be made later on on DBAL?

@michaelperrin

This comment has been minimized.

Show comment
Hide comment
@michaelperrin

michaelperrin Jan 14, 2014

Contributor

Any news on this?
If there are any concerns, I'd be glad to fix them.

Contributor

michaelperrin commented Jan 14, 2014

Any news on this?
If there are any concerns, I'd be glad to fix them.

Show outdated Hide outdated lib/Doctrine/ORM/Mapping/DefaultQuoteStrategy.php
return isset($class->table['quoted'])
? $platform->quoteIdentifier($class->table['name'])
: $class->table['name'];
if ( ! empty($class->table['schema'])) {

This comment has been minimized.

@Ocramius

Ocramius Jan 14, 2014

Member

Can you switch all these to avoid excessive else?

Something like:

$foo = 'bar';

if ($condition) {
    $foo = 'baz';

    if ($otherCondition) { 
        $foo = 'tab';
    }
}
@Ocramius

Ocramius Jan 14, 2014

Member

Can you switch all these to avoid excessive else?

Something like:

$foo = 'bar';

if ($condition) {
    $foo = 'baz';

    if ($otherCondition) { 
        $foo = 'tab';
    }
}

This comment has been minimized.

@michaelperrin

michaelperrin Jan 15, 2014

Contributor

Concerning this one, I agree with you about code readability but performance will be slightly affected.
However I think that readability wins on this!

@michaelperrin

michaelperrin Jan 15, 2014

Contributor

Concerning this one, I agree with you about code readability but performance will be slightly affected.
However I think that readability wins on this!

Show outdated Hide outdated lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php
$sequenceBaseName = $class->getTableName();
// Prepend the schema name to the table name if there is one
if ($schemaName = $class->getSchemaName()) {

This comment has been minimized.

@Ocramius

Ocramius Jan 14, 2014

Member

Can you move this logic to a protected method? The method name in the call would make it clearer and you'd get rid of the inline comment as well.

@Ocramius

Ocramius Jan 14, 2014

Member

Can you move this logic to a protected method? The method name in the call would make it clearer and you'd get rid of the inline comment as well.

This comment has been minimized.

@Ocramius

Ocramius Jan 14, 2014

Member

Actually, private is better

@Ocramius

Ocramius Jan 14, 2014

Member

Actually, private is better

Show outdated Hide outdated lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php
*
* @return string
*/
protected function getSequenceName(ClassMetadataInfo $class)

This comment has been minimized.

@Ocramius

Ocramius Jan 14, 2014

Member

private would be better imo

@Ocramius

Ocramius Jan 14, 2014

Member

private would be better imo

Show outdated Hide outdated lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php
*/
public function getSchemaName()
{
if (isset($this->table['schema'])) {

This comment has been minimized.

@Ocramius

Ocramius Jan 14, 2014

Member

return isset($this->table['schema']) ? $this->table['schema'] : null

Nitpicking - if you don't like this, then ignore it

@Ocramius

Ocramius Jan 14, 2014

Member

return isset($this->table['schema']) ? $this->table['schema'] : null

Nitpicking - if you don't like this, then ignore it

Show outdated Hide outdated lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php
if ($schemaName = $class->getSchemaName()) {
$sequenceBaseName = $schemaName . '.' . $sequenceBaseName;
if ( ! $this->targetPlatform->supportsSchemas() && $this->targetPlatform->canEmulateSchemas()) {

This comment has been minimized.

@Ocramius

Ocramius Jan 14, 2014

Member

@deeky666 what happens when a platform does not support schemas yet cannot emulate them? Does this simply explode at db level?

@Ocramius

Ocramius Jan 14, 2014

Member

@deeky666 what happens when a platform does not support schemas yet cannot emulate them? Does this simply explode at db level?

This comment has been minimized.

@deeky666

deeky666 Jan 14, 2014

Member

@Ocramius That is actually a good question. Currently $this->targetPlatform->usesSequenceEmulatedIdentityColumns() only applies to PostgreSQL and Oracle. PostgreSQL uses native schemas (schema.table_column_seq) and Oracle neither has native schema support, nor emulates schemas. See here.
But I agree, as long as the double underscored notation is not a convention by Doctrine used hardcoded all over the place, I would delegate the sequence name generation to the platform!

@deeky666

deeky666 Jan 14, 2014

Member

@Ocramius That is actually a good question. Currently $this->targetPlatform->usesSequenceEmulatedIdentityColumns() only applies to PostgreSQL and Oracle. PostgreSQL uses native schemas (schema.table_column_seq) and Oracle neither has native schema support, nor emulates schemas. See here.
But I agree, as long as the double underscored notation is not a convention by Doctrine used hardcoded all over the place, I would delegate the sequence name generation to the platform!

Show outdated Hide outdated lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php
$sequenceBaseName = $schemaName . '.' . $sequenceBaseName;
if ( ! $this->targetPlatform->supportsSchemas() && $this->targetPlatform->canEmulateSchemas()) {
$sequenceBaseName = str_replace('.', '__', $sequenceBaseName);

This comment has been minimized.

@Ocramius

Ocramius Jan 14, 2014

Member

I think schema emulation should happen in the platform directly - assuming str_replace here seems rather simplistic/fragile

@Ocramius

Ocramius Jan 14, 2014

Member

I think schema emulation should happen in the platform directly - assuming str_replace here seems rather simplistic/fragile

Show outdated Hide outdated lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php
@@ -1966,6 +1966,20 @@ public function getTableName()
}
/**
* Gets primary table's shema name.
*
* @return string

This comment has been minimized.

@Ocramius

Ocramius Jan 14, 2014

Member

string|null

@Ocramius

Ocramius Jan 14, 2014

Member

string|null

@michaelperrin

This comment has been minimized.

Show comment
Hide comment
@michaelperrin

michaelperrin Jan 15, 2014

Contributor

@Ocramius I made most of the changes you mentioned.
There are now an extra private method to get the sequence name, and some small code changes have been made accordingly to your comments.

I updated the documentation for all mapping formats. It was not necessary to update the XSD file as it already mentioned the schema attribute.

I agree with you concerning the use of double underscores directly in the ORM, it should be handled directly by platforms in DBAL.

However, do you think we consider this PR as a first step?

I can then open a PR on DBAL to add a method like AbstractPlatform::getSequenceName($columnName, $tableName, $schemaName = null) and make use of it in the ORM.
An other method like this should also be implemented for getting the table name depending on the schema name and table name in DBAL. It would be used in the ORM in DefaultQuoteStrategy::getTableName.

Do you want me to do this already now, or after this PR is merged?

Contributor

michaelperrin commented Jan 15, 2014

@Ocramius I made most of the changes you mentioned.
There are now an extra private method to get the sequence name, and some small code changes have been made accordingly to your comments.

I updated the documentation for all mapping formats. It was not necessary to update the XSD file as it already mentioned the schema attribute.

I agree with you concerning the use of double underscores directly in the ORM, it should be handled directly by platforms in DBAL.

However, do you think we consider this PR as a first step?

I can then open a PR on DBAL to add a method like AbstractPlatform::getSequenceName($columnName, $tableName, $schemaName = null) and make use of it in the ORM.
An other method like this should also be implemented for getting the table name depending on the schema name and table name in DBAL. It would be used in the ORM in DefaultQuoteStrategy::getTableName.

Do you want me to do this already now, or after this PR is merged?

@Ocramius

This comment has been minimized.

Show comment
Hide comment
@Ocramius

Ocramius Jan 15, 2014

Member

@michaelperrin I think that's up to @deeky666 to decide. DBAL is already in beta, so I'm not sure if it can be added now without messing with the release process.

Is the current failure on travis related with your changes?

Member

Ocramius commented Jan 15, 2014

@michaelperrin I think that's up to @deeky666 to decide. DBAL is already in beta, so I'm not sure if it can be added now without messing with the release process.

Is the current failure on travis related with your changes?

@michaelperrin

This comment has been minimized.

Show comment
Hide comment
@michaelperrin

michaelperrin Jan 15, 2014

Contributor

@Ocramius No, it's not related to my changes. I think it's doctrine/dbal#508 on DBAL that causes Travis failure.

Contributor

michaelperrin commented Jan 15, 2014

@Ocramius No, it's not related to my changes. I think it's doctrine/dbal#508 on DBAL that causes Travis failure.

@deeky666

This comment has been minimized.

Show comment
Hide comment
@deeky666

deeky666 Jan 15, 2014

Member

@Ocramius The failing test is indeed related to the latest DBAL merge. That will be fixed in #910
I think it will be fine to do it this way right now as I don't know whether those hardcoded sequence names exist in other places, too. It would be too much right now to fix that accordingly and should be delayed to a subsequent PR:

Member

deeky666 commented Jan 15, 2014

@Ocramius The failing test is indeed related to the latest DBAL merge. That will be fixed in #910
I think it will be fine to do it this way right now as I don't know whether those hardcoded sequence names exist in other places, too. It would be too much right now to fix that accordingly and should be delayed to a subsequent PR:

@Ocramius

This comment has been minimized.

Show comment
Hide comment
@Ocramius

Ocramius Jan 15, 2014

Member

@michaelperrin so let's at least introduce // @todo annotations where this has to happen

Member

Ocramius commented Jan 15, 2014

@michaelperrin so let's at least introduce // @todo annotations where this has to happen

@michaelperrin

This comment has been minimized.

Show comment
Hide comment
@michaelperrin

michaelperrin Jan 15, 2014

Contributor

@Ocramius Alright, so adding @todo Sequence names should be computed in DBAL depending on the platform on my ClassMetadataFactory:getSequencePrefix() and ClassMetadataFactory:getSequenceName() methods would be fine?

Contributor

michaelperrin commented Jan 15, 2014

@Ocramius Alright, so adding @todo Sequence names should be computed in DBAL depending on the platform on my ClassMetadataFactory:getSequencePrefix() and ClassMetadataFactory:getSequenceName() methods would be fine?

@Ocramius

This comment has been minimized.

Show comment
Hide comment
@Ocramius
Member

Ocramius commented Jan 15, 2014

@michaelperrin yes please

@michaelperrin

This comment has been minimized.

Show comment
Hide comment
@michaelperrin
Contributor

michaelperrin commented Jan 15, 2014

@Ocramius done

@Ocramius

This comment has been minimized.

Show comment
Hide comment
@Ocramius

Ocramius Jan 15, 2014

Member

👍

Member

Ocramius commented Jan 15, 2014

👍

$tableName = $class->table['schema'] . '.' . $class->table['name'];
if ( ! $platform->supportsSchemas() && $platform->canEmulateSchemas()) {
$tableName = $class->table['schema'] . '__' . $class->table['name'];

This comment has been minimized.

@deeky666

deeky666 Jan 15, 2014

Member

This should be delegated to the platform, too unless it is a fixed Doctrine convention. Therefore a @todo should be added here also IMO.

@deeky666

deeky666 Jan 15, 2014

Member

This should be delegated to the platform, too unless it is a fixed Doctrine convention. Therefore a @todo should be added here also IMO.

This comment has been minimized.

@Ocramius

Ocramius Jan 14, 2015

Member

I will add the @todo on merge

@Ocramius

Ocramius Jan 14, 2015

Member

I will add the @todo on merge

This comment has been minimized.

@michaelperrin

michaelperrin Jan 14, 2015

Contributor

@Ocramius I am still alive to help on this PR and make it possible to merge it (I didn't see @deeky666 's note before). So it's not a problem if you want me to make some changes!

@michaelperrin

michaelperrin Jan 14, 2015

Contributor

@Ocramius I am still alive to help on this PR and make it possible to merge it (I didn't see @deeky666 's note before). So it's not a problem if you want me to make some changes!

This comment has been minimized.

@Ocramius

Ocramius Jan 14, 2015

Member

Actually, the @todo is at docblock level

@Ocramius

Ocramius Jan 14, 2015

Member

Actually, the @todo is at docblock level

This comment has been minimized.

@michaelperrin

michaelperrin Jan 14, 2015

Contributor

@Ocramius Which is alright, isn't it?

@michaelperrin

michaelperrin Jan 14, 2015

Contributor

@Ocramius Which is alright, isn't it?

This comment has been minimized.

@Ocramius
@Ocramius

This comment has been minimized.

@Ocramius

Ocramius Jan 14, 2015

Member

I am still alive to help on this PR and make it possible to merge it

I actually pulled all changes locally for merge, so expect it to be merged in the next hours

@Ocramius

Ocramius Jan 14, 2015

Member

I am still alive to help on this PR and make it possible to merge it

I actually pulled all changes locally for merge, so expect it to be merged in the next hours

This comment has been minimized.

@michaelperrin

michaelperrin Jan 14, 2015

Contributor

I actually pulled all changes locally for merge, so expect it to be merged in the next hours

Great! Thank you! I should really consider making some other contributions to Doctrine.

@michaelperrin

michaelperrin Jan 14, 2015

Contributor

I actually pulled all changes locally for merge, so expect it to be merged in the next hours

Great! Thank you! I should really consider making some other contributions to Doctrine.

@deeky666

This comment has been minimized.

Show comment
Hide comment
@deeky666

deeky666 Jan 15, 2014

Member

Looks good to me now :)

Member

deeky666 commented Jan 15, 2014

Looks good to me now :)

@michaelperrin

This comment has been minimized.

Show comment
Hide comment
@michaelperrin

michaelperrin Jan 15, 2014

Contributor

@deeky666 You are right concerning the @todo comment, I have added it there to.

Contributor

michaelperrin commented Jan 15, 2014

@deeky666 You are right concerning the @todo comment, I have added it there to.

@michaelperrin

This comment has been minimized.

Show comment
Hide comment
@michaelperrin

michaelperrin Feb 10, 2014

Contributor

Can this PR be merged?
The Travis error was not due to the PR itself, but because the master branch of Doctrine could not be built when the PR was proposed. I don't know how I can re-run the Travis build for this PR.

Contributor

michaelperrin commented Feb 10, 2014

Can this PR be merged?
The Travis error was not due to the PR itself, but because the master branch of Doctrine could not be built when the PR was proposed. I don't know how I can re-run the Travis build for this PR.

@michaelperrin

This comment has been minimized.

Show comment
Hide comment
@michaelperrin

michaelperrin Apr 17, 2014

Contributor

@guilhermeblanco There are a lot of little things to be careful about indeed. I think this PR solves (most?) problems, at least for platforms that don't support schemas like SQLite but with entities declared with a schema (so that that Doctrine remains cross-platform) as it is currently impossible to persist an entity.
Do you think I should add more tests on some specific parts?

Contributor

michaelperrin commented Apr 17, 2014

@guilhermeblanco There are a lot of little things to be careful about indeed. I think this PR solves (most?) problems, at least for platforms that don't support schemas like SQLite but with entities declared with a schema (so that that Doctrine remains cross-platform) as it is currently impossible to persist an entity.
Do you think I should add more tests on some specific parts?

@craaazy19

This comment has been minimized.

Show comment
Hide comment
@craaazy19

craaazy19 Aug 11, 2014

Hello guys, would you merge this request? There is a real problem with sqlite because of this bug.

craaazy19 commented Aug 11, 2014

Hello guys, would you merge this request? There is a real problem with sqlite because of this bug.

@dwendelen

This comment has been minimized.

Show comment
Hide comment
@dwendelen

dwendelen Sep 6, 2014

I need this feature as well. I would like to use in-memory sqlite DB for scenario-tests.

dwendelen commented Sep 6, 2014

I need this feature as well. I would like to use in-memory sqlite DB for scenario-tests.

@richardfullmer

This comment has been minimized.

Show comment
Hide comment
@richardfullmer

richardfullmer Sep 12, 2014

Member

👍

Also running Postgresql as the production database with a desire to run SQLite for quick testing.

Member

richardfullmer commented Sep 12, 2014

👍

Also running Postgresql as the production database with a desire to run SQLite for quick testing.

@Ocramius

This comment has been minimized.

Show comment
Hide comment
@Ocramius

Ocramius Nov 11, 2014

Member

@deeky666 do we have anything to abstract away the schema stuff now?

Member

Ocramius commented Nov 11, 2014

@deeky666 do we have anything to abstract away the schema stuff now?

@Ocramius Ocramius changed the title from Fix persistence on a table with a schema on a platform without schema support to [DDC-2825] Fix persistence on a table with a schema on a platform without schema support Jan 14, 2015

Ocramius added a commit that referenced this pull request Jan 14, 2015

Ocramius added a commit that referenced this pull request Jan 14, 2015

Ocramius added a commit that referenced this pull request Jan 14, 2015

Ocramius added a commit that referenced this pull request Jan 14, 2015

#881 DDC-2825 - moving YAML and XML mapping tests to base mapping dri…
…ver tests. Excluding Static PHP mapping tests

Ocramius added a commit that referenced this pull request Jan 14, 2015

Ocramius added a commit that referenced this pull request Jan 14, 2015

Ocramius added a commit that referenced this pull request Jan 14, 2015

#881 DDC-2825 - refactoring mapping driver to use `ClassMetadata#setP…
…rimaryTable()` instead of duplicating `explode()` logic

Ocramius added a commit that referenced this pull request Jan 14, 2015

Ocramius added a commit that referenced this pull request Jan 14, 2015

Ocramius added a commit that referenced this pull request Jan 14, 2015

Ocramius added a commit that referenced this pull request Jan 14, 2015

Ocramius added a commit that referenced this pull request Jan 14, 2015

Ocramius added a commit that referenced this pull request Jan 14, 2015

@Ocramius Ocramius closed this in 1d4b96e Jan 14, 2015

@Ocramius

This comment has been minimized.

Show comment
Hide comment
@Ocramius

Ocramius Jan 14, 2015

Member

I rewrote large chunks of the testing in this PR, as I found various bugs with XML/YAML and PHP mapping types (they were untested).

Other than that, this is merged via 1d4b96e and will land in 2.5.

Thanks @michaelperrin!

Member

Ocramius commented Jan 14, 2015

I rewrote large chunks of the testing in this PR, as I found various bugs with XML/YAML and PHP mapping types (they were untested).

Other than that, this is merged via 1d4b96e and will land in 2.5.

Thanks @michaelperrin!

@Ocramius Ocramius self-assigned this Jan 14, 2015

@michaelperrin

This comment has been minimized.

Show comment
Hide comment
@michaelperrin

michaelperrin Jan 14, 2015

Contributor

Thanks @Ocramius! Glad this PR got merged and I'm looking forward to the future 2.5 release and other contributions as well!

Contributor

michaelperrin commented Jan 14, 2015

Thanks @Ocramius! Glad this PR got merged and I'm looking forward to the future 2.5 release and other contributions as well!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment