Increasing the severity and adding a fix version since this MUST be fixed before next release.

I find this to be of rather minor importance. You're talking of compatibility breaks, but thats only the case if we intend to start being very nitpicky about the naming in the future. We are currently not, and we wont be, so not much risk of later breakage.

We have a rule of thumb already: Acronyms up to 4 characters all uppercase, otherwise normal camelcase.

Now, it is often a matter of taste what is an acronym and what not and also not all acronyms have a clear casing, prime example mysql.

Being too nit-picky here is just a pain in the ass and we won't reach a common consensus anyway.

Oh and we better dont start arguing about whats better to read because there is no chance of agreement. If you ask me I find SQLException better than SqlException. So here we go. Lets better not run down this path.

No need to get upset, I'm only trying to help.

I just thought that consistency is usually a good idea, either way.

I have to disagree in that an acronym is an acronym, it's not a matter of taste, the letters either stand for something, or they don't.

As for "mysql", only the SQL part is an acronym. So, MySQL or MySql.

Close if you disagree.

I'm not upset. What made you think so? Maybe I should introduce a every now and then.

There's just no point in arguing about readability.

Like I said, we do have a naming standard even if its not adhered everywhere. The standard is also not something we've made up ourselves because we try to avoid that. When we introduced namespaces, we talked about adopting either the Java or .NET naming standards. We opted for the .NET standards. And there its recommended to write acronyms with up to 4 characters all uppercase.

I dont think there are too many violations of that in the code, probably Cli => CLI and some others but most of it looks ok to me.

UPDATE: Maybe we can gather a list here in this ticket of violations? Cli => CLI would be one. MySql => MySQL another. "Id" is rather an abbreviation of Identifier and not an acronym, to me at least.

It's not a case of getting anyone upset...

But we should fix it and keep consistency of our codebase.

@romanb We should all talk and reach a common sense.
That's our last chance (last alpha) to get this consistency in.

@Guilherme: We do have a naming standard since a year. You want to change the standard now?

@Roman,

Just a feeling I got.

This issue was more about consistency (indicated in the title) than moving to proper-case naming.

I think it might be up to 3 characters in .NET, HtmlElement, System.Linq, etc. Anyway, not important.

I agree that Id. is an abbreviation.

There are some more violations. If you decide you want to change them, let me know and I'll compile a list.

@Glen: Yes, a list would be great. I find it hard to be 100% consistent sometimes though because my taste conflicts with the rule. For example, I would prefer "Id" over "ID", especially since it comes directly after ORM "Doctrine\ORM\ID\..." would be a bit too much. But I would not like "Orm" or "Dbal" either. But I think in most cases we can clearly fix the inconsistency. Like CLI or MySQL or MSSQL (or should it be MsSQL?).

Thanks for your help!

We should probably create updated coding standards for Doctrine 2 also, since they differ quite some from Doctrine 1 due to the introduction of namespaces and such. I'll create a ticket for that.

I'll do the list for you by tomorrow at the latest, just running out of time ATM.

Id is correct, as mentioned above, so that would be fine. MsSQL is correct (Ms = Microsoft = abbreviation).

Found some time ...

Doctrine\Common\Cache\ApcCache -> APCCache
Doctrine\Common\Cli -> CLI
Doctrine\Common\Cli\Printers\AnsiColorPrinter -> ANSIColorPrinter
Doctrine\Common\Cli\CliController -> CLIController
Doctrine\Common\Cli\CliException -> CLIException

Doctrine\DBAL\Driver\PDOMsSql -> PDOMsSQL
Doctrine\DBAL\Driver\PDOMySql -> PDOMySQL
Doctrine\DBAL\Driver\PDOPgSql -> PDOPgSQL
Doctrine\DBAL\Driver\PDOSqlite -> PDOSQLite
Doctrine\DBAL\Logging\EchoSqlLogger -> EchoSQLLogger
Doctrine\DBAL\Logging\SqlLogger -> SQLLogger
Doctrine\DBAL\Platforms\MsSqlPlatform -> MsSQLPlatform
Doctrine\DBAL\Platforms\MySqlPlatform -> MySQLPlatform
Doctrine\DBAL\Platforms\PostgreSqlPlatform -> PostgreSQLPlatform
Doctrine\DBAL\Platforms\SqlitePlatform -> SQLitePlatform
Doctrine\DBAL\Schema\Visitor\CreateSchemaSqlCollector -> CreateSchemaSQLCollector
Doctrine\DBAL\Schema\Visitor\DropSchemaSqlCollector -> DropSchemaSQLCollector
Doctrine\DBAL\Schema\MsSqlSchemaManager -> MsSQLSchemaManager
Doctrine\DBAL\Schema\MySqlSchemaManager -> MySQLSchemaManager
Doctrine\DBAL\Schema\PostgreSqlSchemaManager -> PostgreSQLSchemaManager
Doctrine\DBAL\Schema\SqliteSchemaManager -> SQLiteSchemaManager
Doctrine\DBAL\Tools\Cli -> CLI
Doctrine\DBAL\Tools\Cli\Tasks\RunSqlTask -> RunSQLTask

Doctrine\ORM\Mapping\Driver\PhpDriver -> PHPDriver
Doctrine\ORM\Mapping\Driver\XmlDriver -> XMLDriver
Doctrine\ORM\Mapping\Driver\YamlDriver -> YAMLDriver
Doctrine\ORM\Query\Exec\AbstractSqlExecutor -> AbstractSQLExecutor
(Should Doctrine\ORM\Query\Expr\Andx and Orx be AndX and OrX?)
Doctrine\ORM\Query\SqlWalker -> SQLWalker
Doctrine\ORM\Tools\Cli -> CLI
Doctrine\ORM\Tools\Cli\Tasks\RunDqlTask -> RunDQLTask
Doctrine\ORM\Tools\Export\Driver\PhpExporter -> PHPExporter
Doctrine\ORM\Tools\Export\Driver\XmlExporter -> XMLExporter
Doctrine\ORM\Tools\Export\Driver\YamlExporter -> YAMLExporter

... I think that's it. More than you expected? I did say: "There is more proper-casing than upper-casing."

No, not more than I expected. It's mostly SQL and CLI basically. Thanks for the list!

We should try to go through that list before beta.

Methods should be adjusted accordingly also (even though they're case insensitive). I already started with that. More to come.

Most of the Cli => CLI changes seem to be complete now.

Pushing the rest of the name changes towards beta2.

More renamings have been done but still more to do. Pushing remaining work to beta3.

Final name changes should be done prior to going into RC1.

there is much to be done yet, however most of it is also public API class names and might cause quite some BC breaks (i.e. DBAL Platforms and Schema Manager, Mapping Driver Names). I am not sure how to proceed here.

Since PHP is mostly case-insensitive on class and method names it shouldn't be much of an issue.

Has there been any work on a coding standards document yet?
I'm currently working on fixing documentation on this project, and it might be a good time to define a standard.
I've started compiling a few recommendations based on various feedbacks I've got in my pull requests, and I can post them here.
Please let me know if there have been previous attempts so far!

Benjamin Morel Guilherme Blanco may have a CS ruleset, but it's not ready yet. Perfect timing btw, we really need to automate this to avoid having all these useless CS fix comments in pull requests

Ok, I'll post my document here once ready, and Guilherme Blanco will be able to compare it with his ruleset!

Here is a first draft: https://gist.github.com/4676670

Please comment!

Guilherme Blanco, if you don't have time to compare your ruleset with my draft, maybe you could publish your current ruleset so that others can have a look?

i just found a possible BC issue with this.

EntityRepository is allowed to be extended by us, it has several variables that are underscore prefixed. How to proceed in this case?

I know but its not really a problem I think. We should just decide whether we make them private in the first place and provide getters instead (which would have avoided this problem in the first place).

Leaving the prefixes on the repository class only is also an option... but I dont think thats necessary.

can we commit getters for Beta 1 then? We could give everyone a period until Beta 2 to fix their code and then make the change.

EntityRepository is the only class that is meant to be userland extendable to my knowledge, so this should be the only problem to adress

Yes, you can add getters and commit right away if you want. Plus adding a note on the upgrade document that direct access of these properties is deprecated.

Persisters will be also extensible some day in userland but they need more polish for that, I've already started with it

Is this still planned? Searching the code base finds this is not being implemented. It would be a good idea to implement the change sooner than later if it will be done at all. Also, +1 for the change. It makes complete sense.

Apparently it's also incompatible with several other tag as well.

I thought it made sense to try the following and see if the @InheritanceType and @Discriminator___ tags would apply to the children classes:

/**
 * @MappedSuperclass
 * @InheritanceType("SINGLE_TABLE")
 * @DiscriminatorColumn(name="type", type="string")
 * @DiscriminatorMap({"User" = "User", "Group" = "Group"})
 */
abstract class Principal

But apparently this flags D2 to treat it as an Entity anyway, resulting in the following error:

PDOException: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'sentact5.principal'

I updated the documentation, the question is if we should check for the mapped superclass attribute and throw exceptions if other entity level annotations are specified.

A mapped superclass has not many restrictions and these are mentioned in the docs (i.e. only unidirectional associations), what David mentions above should work, if it doesnt its a bug, I think DDC-511 looks like that same issue.

David,

@"Is this defined on the superclass or on the children classes?"

It doesnt matter. A @MappedSuperclass can be anywhere in an inheritance hierarchy and it always does the same thing, inherit its mapping information to subclasses (but its not itself an entity). The docs say:

Mapped superclasses, just as regular, non-mapped classes, can appear in the middle of an otherwise mapped inheritance hierarchy (through Single Table Inheritance or Class Table Inheritance).

as well as

Entities support inheritance, polymorphic associations, and polymorphic queries. Both abstract and concrete classes can be entities. Entities may extend non-entity classes as well as entity classes, and non-entity classes may extend entity classes.

So entities, mapped superclasses and plain non-mapped classes can appear mixed in an inheritance hierarchy. Nevertheless all the classes in a hierarchy that are entities must use 1 inheritance strategy, you can not mix inheritance mapping strategies in a single class hierarchy.

@"If it's defined on the child classes, does it take parameters? The name of the super class?"

No, it doesnt. The docs dont mention any parameters either which is correct.

@"It was not at all apparent to me that it was mutually exclusive with the @Entity tag"

OK, that needs to be made clearer in the docs then.

Irrelevant

It looks like there is an even broader problem with the new _CG_ prefix; the PSR-0 standard for autoloading states that the underscores should be handled this way:

\namespace\package\Class_Name => {...}/namespace/package/Class/Name.php

Which means that in the above example, it could even expect the file to be located in:

Application/Proxy///CG///Application/Model/User.php

... which is far away from the actual location.
Upgrade to 2.2 broke this code, for us.

Proxy classes do not follow PSR-0. For the case unserializing objects we should provide an extra autoloader i guess.

See here how symfony does it https://github.com/doctrine/DoctrineBundle/blob/master/DoctrineBundle.php#L57

See https://github.com/doctrine/doctrine2/commit/9b4d60897dfc7e9b165712428539e694ec596c80 and https://github.com/doctrine/orm-documentation/commit/01381fae1ff3d4944086c7cfe46721925bf6ca15

Thanks for the quick fix, Benjamin.
However, I have to admit that I'm not fully happy with the fix, as we (and probably many others) are not using the Doctrine autoloader.
I supposed that the purpose of PSR-0 was precisely not to be tied to a particular autoloader implementation, and this benefit is lost with this version of Doctrine.

You mentioned in the doc that the proxies are not PSR-0 compliant "for implementation reasons"; as this was working fine before 2.2, could you please explain what requirement prevents Doctrine from keeping the previous naming convention?

In 2.1 the proxies are not PSR-0 compatible themselves, however their class naming is simpler.

In 2.2 we changed proxy names so that you can derive the original name of the proxy by searching for the _CG_ flag. This flag obviously contains the __ chars that some PSR autoloaders detect as directory seperators. I agree this is an unfortunate decision, but it was done this way.

I do think however that we can automatically register the proxy atuoloader (if not yet done) in EntityManager#create(). This would hide this fact from developers automatically.

@Benjamin Eberlei
In 2.3 we still have to manually call Autoloader::register() before unserializing entities that may contain proxies.
So EntityManager::create() still doesn't register it. Is there a plan to add this feature?

Benjamin Morel Not at the moment, seems too dangerous for me since it might produce race conditions. This should really be done in the bootstrap of the system.

We need to document this though.

Ok, thanks for your answer!

Debian Wheezy, Mysql 5.1, Apache2, PHP 5.4

A little up... for inspiration from JPA

http://en.wikibooks.org/wiki/Java_Persistence/OneToMany#Undirectional_OneToMany.2C_No_Inverse_ManyToOne.2C_No_Join_Table_.28JPA_2.0_ONLY.29

Hi all!
Maybe if this task is hard, you could do a simplier variant, "FROM Class1 a1 JOIN a1,a2 WITH a2 INSTANCE OF Class2"?
Doctrine 2.3 supports it, but in fact, it does not work. I can't use Class2 fields in query, and Doctrine ignores the INSTANCE OF operator when building a native queries.
I am working with system where I have many inherited classes with Class Table Inheritance. When I do a JOIN query, it generates native sql query more than 1KB(?!) length, and MySQL freezes for more than 7 minutes (?!) on it. It must join only tables for Class2, but it joins all my 20 tables inherited of my base class
I don't know where to send bugreport
It will be very good if I could manually select a joined class to make Doctrine do a better queries.
Sorry for my english, I am from Moscow.

onFluish is not the start of a transaction. It has nothing to do with this.

Is a third event needed? Or is this to be marked as "won't fix"?

Maybe onBeginTransaction, onCommit and onRollback.

However since you can start transactions manually using $em->beginTransaction(), the Flush events are somehwat independent of transactions anyways.

Well, user can start transaction anytime, but the fact is that if we think ORM we do not know nothing about the database. we just persist and flush objects.

Yes I think these would be very useful, from how I see it, if you use event listeners, is:

loadClassMetadata: you can apply extra mapping

onFlush: you can modify entity changesets, or persist recalculate new ones, without triggering the database, since it is not used to begin the database modifications yet.

onBeginTransaction: could use the database modifications keeping in sync the entity changesets. the thing about this event is that usually in behavioral way atomic updates are required. for example nestedset tree sync lft rgt columns, sortable sync the sort index, materialized path, all these requires atomic updates, and the best place is the start of transaction.

onCommit: could be useful to execute right before commit, finalizing database modifications could be done.

onRollback: this one is really something, since if you go far, there might be something like files uploaded during the entity processing, and you may want to remove them if transaction fails.

This situation was barely documented here: http://www.doctrine-project.org/jira/browse/DDC-1443

We need a better Transaction API that completely fixes the computation of changesets and also allow more fine grained control over Entities and their corresponding information.

I'd postpone this one until 3.0.

(I have no Link Privileges, but this one #DDC-1379 is a duplicate with more extent info.)

Existing PR at https://github.com/doctrine/doctrine2/pull/361

Not a bug

I've added two more cases where the parsing fails. Do you want a separate tickets for that?

Don't worry, I'll spend some time over this...
But I'm not sure about the last one.

The 3rd case seems work just fine as a part of a HAVING clause.
I haven't tried it but It might be that it fails with something simpler like SELECT COUNT( * ) = :foo FROM ... or SELECT COUNT( * ) = 2 FROM ...

Fabio I have two more...
It doesn't like NULL and subselect after then part

 
->addSelect('CASE
    WHEN po.quantity IS NULL THEN NULL
    ELSE po.quantity -
            COALESCE(0, (
                SELECT COUNT(rd.product) FROM xxxx rd
                    WHERE (rd.startDate <= :end) AND (rd.endDate >= :start) AND
                        rd.product = c.product)))
    END
    AS po.quantity
')

:edit replaced with real query

addon: well the subquery part can be full query with joins ....

Hi

Could you please attach your entities or a failing test case ?

Cheers

sure

LineRelated.php :

class LineRelated
{
    /**
     * @ORM\Id
     * @ORM\Column(type="integer")
     * @ORM\GeneratedValue(strategy="AUTO")
     */
     protected $id;

    /**
     * @ORM\ManyToOne(targetEntity="Line", inversedBy="line_related")
     * @ORM\JoinColumn(name="line_id", referencedColumnName="id",nullable=false)
     */
     protected $line;
     
    /**
     * @ORM\Column(name="line_id_related", type="integer")
     */
    protected $line_related;

    /**
     * @ORM\Column(type="smallint",nullable=false)
     */
     protected $count = 0;

###### get/set etc....... #######

Line.php

class Line
{
    /**
     * @ORM\Id
     * @ORM\Column(type="integer")
     * @ORM\GeneratedValue(strategy="AUTO")
     */
    protected $id;
########## blablabla #############

my action:

    public function right_line_relatedAction($line = null, $title='相关线路')
    {

        $lines = $l->getByUser($user, array());
//anyway,$lines is an array,It has several elements,each element is an instance of LineEntity.

        $related = $this->getDoctrine()->getRepository('MyWebBundle:LineRelated')->findBy(array('line' => $lines), array('count' => 'DESC'), 20);
//this findBy function generate the sql which is slow.

        return $related;
    }

Hi,

How did you get this query string ?

Repository#findBy does not quote the values, It uses PDO:bindParam.
so the expected query string should be someting like :

WHERE t0.line_id IN (?, ? ,?) 

I tried to reproduce but in my tests the generated Query binds the parameters as "PDO::PARAM_INT".

I have added a test case.
Could you please can try to change it and make fails.

Cheers

reproduced :

SELECT t0.id AS id1, t0.line_id_related AS line_id_related2, t0.count AS count3, t0.line_id AS line_id4 FROM line_related t0 WHERE t0.line_id IN ('6059', 4851, '6068', 126352, '6060', '1000000') ORDER BY t0.count DESC LIMIT 20
Parameters: [['6059', 4851, '6068', 126352, '6060', '1000000']] 
[Hide runnable query]
Time: 234.53 ms [   Explain query ]

let me have a look on what's going on

interesting. I've dump(using ladybug_dump) the $lines,and I found out that when the element is a Proxies Object(Object(Proxies_GC_\My\WebBundle\Entity\Line)),then the id of that Object will be with quoted,when the elememt is an Real Entity,then It will be without quote.

for example,in my last comment, the parameters is [['6059', 4851, '6068', 126352, '6060', '1000000']]
the result of dumping $lines is :

array(6)
[0]: object(Proxies_CG_\Zuo\WebBundle\Entity\Line)
[1]: object(Zuo\WebBundle\Entity\Line)
[2]: object(Proxies_CG_\Zuo\WebBundle\Entity\Line)
[3]: object(Zuo\WebBundle\Entity\Line)
[4]: object(Proxies_CG_\Zuo\WebBundle\Entity\Line)
[5]: object(Proxies_CG_\Zuo\WebBundle\Entity\Line)

tell me if you need more information. thanks

This may be because

$_identifier

in proxies ( https://github.com/doctrine/doctrine2/blob/42e83a2716d19eada4f1cd49ece77d5f5229a239/lib/Doctrine/ORM/Proxy/ProxyFactory.php#L383 ) is not necessarily composed by integers. This could be fixed with DCOM-96. I'll add the tests to my development branch and will ping you back tomorrow

thanks

I see what is going on here... But this should not be a problem anyway, since they're bound anyway as "PDO::PARAM_INT", as Fabio B. Silva told you.

That's only a problem with the logger showing them as string. PDO will handle the conversion before the value hits the DB as far as I know.

I can understand your point,but what I don't really get is that the execute time of sql is very long,that explained the quote should be in the sql,not like what you said,that's only a problem with the logger.

scourgen can you profile the difference directly in CLI? What about checking the bound parameter type? Are those values bound as INTs in your case?

@ocramius I wish I could, but I was using doctrine2 with symfony2,So It looks like It will takes some time to simulating all environment and settings that could allow me to reproduced the problem.

but anyway,I will have a try and tell you what happen when I found something.

scourgen ok, awaiting your reply then

I've spent some time on playing with native doctrine2. It took me awhile to setup everything. but I just don't get that how to retrive data with its Proxy ojbect(for example Proxies_CG_\My\WebBundle\Entity\Line).

I mean the result of

$this->_em->getRepository("something")->findxxx()

always return an array of real object. I can't reproduced the situation(#comment-19186) that happens on symfony2+doctrine2.

anyway,I can make sure the problem is real exist,Because the execute time of that slow sql from the tool bar of symfony2 is same as I executed it at mysql cli. If the sql shows up on log with quote but running at mysql without quote,the execute time won't be same(actually It will be much more faster,in my case,20x times,from 2xxms to 10ms).

scourgen you can use

$em->getReference($className, $identifier)

(identifier being a key=>value array) to force proxies.

Give it a try

looks like I reproduced it.

    public function testIssue()
    {   
         $no_used=   $this->_em->getRepository(__NAMESPACE__. '\DDC2214Line')->findOneById(1);
        $lines=array(
            //$this->_em->getRepository(__NAMESPACE__. '\DDC2214Line')->findOneById(1),
            $this->_em->getReference(__NAMESPACE__. '\DDC2214Line',1),
            $this->_em->getReference(__NAMESPACE__. '\DDC2214Line','2'),
            $this->_em->getReference(__NAMESPACE__. '\DDC2214Line',3),
        );  
        $logger  = $this->_em->getConnection()->getConfiguration()->getSQLLogger();
        $ids     = array_map(function($r){
            return $r->id;
        }, $this->relatedList);

        //$related = $this->_em->getRepository(__NAMESPACE__ . '\DDC2214LineRelated')->findBy(array('line' => $lines), array('count' => 'DESC'), 20);
        $related = $this->_em->createQuery('select lr from '.__NAMESPACE__ . '\DDC2214LineRelated lr where lr.id in (:ids)')->setParameter('ids',$lines)->getResult();
            
        $query   = end($logger->queries);
//\Doctrine\Common\Util\Debug::dump($query['params']);

        $this->assertCount(3, $related);
        $this->assertEquals($ids, $query['params'][0]);
        $this->assertEquals(\Doctrine\DBAL\Connection::PARAM_INT_ARRAY, $query['types'][0]);
    }   
}

I use MySql Query log to see what's really happen in database(http://dev.mysql.com/doc/refman/5.5/en/query-log.html)

this is the log from table mysql.general_log

2013-01-08 12:23:44	[root] @ localhost [127.0.0.1]	59	0	Connect	root@localhost on doctrine_tests
2013-01-08 12:23:44	root[root] @ localhost [127.0.0.1]	59	0	Query	CREATE TABLE DDC2214Line (id INT AUTO_INCREMENT NOT NULL, PRIMARY KEY(id)) DEFAULT CHARACTER SET utf8 COLLATE utf8_unicode_ci ENGINE = InnoDB
2013-01-08 12:23:44	root[root] @ localhost [127.0.0.1]	59	0	Query	CREATE TABLE DDC2214LineRelated (id INT AUTO_INCREMENT NOT NULL, line_id INT NOT NULL, count SMALLINT NOT NULL, line_id_related INT NOT NULL, INDEX IDX_D31307994D7B7542 (line_id), PRIMARY KEY(id)) DEFAULT CHARACTER SET utf8 COLLATE utf8_unicode_ci ENGINE = InnoDB
2013-01-08 12:23:44	root[root] @ localhost [127.0.0.1]	59	0	Query	ALTER TABLE DDC2214LineRelated ADD CONSTRAINT FK_D31307994D7B7542 FOREIGN KEY (line_id) REFERENCES DDC2214Line (id)
2013-01-08 12:23:44	root[root] @ localhost [127.0.0.1]	59	0	Query	START TRANSACTION
2013-01-08 12:23:44	root[root] @ localhost [127.0.0.1]	59	0	Query	INSERT INTO DDC2214Line (id) VALUES (null)
2013-01-08 12:23:44	root[root] @ localhost [127.0.0.1]	59	0	Query	INSERT INTO DDC2214Line (id) VALUES (null)
2013-01-08 12:23:44	root[root] @ localhost [127.0.0.1]	59	0	Query	INSERT INTO DDC2214Line (id) VALUES (null)
2013-01-08 12:23:44	root[root] @ localhost [127.0.0.1]	59	0	Query	INSERT INTO DDC2214LineRelated (count, line_id_related, line_id) VALUES (1, 1, 1)
2013-01-08 12:23:44	root[root] @ localhost [127.0.0.1]	59	0	Query	INSERT INTO DDC2214LineRelated (count, line_id_related, line_id) VALUES (2, 2, 2)
2013-01-08 12:23:44	root[root] @ localhost [127.0.0.1]	59	0	Query	INSERT INTO DDC2214LineRelated (count, line_id_related, line_id) VALUES (3, 3, 3)
2013-01-08 12:23:44	root[root] @ localhost [127.0.0.1]	59	0	Query	COMMIT
2013-01-08 12:23:44	root[root] @ localhost [127.0.0.1]	59	0	Query	SELECT t0.id AS id1 FROM DDC2214Line t0 WHERE t0.id = 1 LIMIT 1
2013-01-08 12:23:44	root[root] @ localhost [127.0.0.1]	59	0	Query	SELECT d0_.id AS id0, d0_.count AS count1, d0_.line_id_related AS line_id_related2, d0_.line_id AS line_id3 FROM DDC2214LineRelated d0_ WHERE d0_.id IN (1, '2', 3)
2013-01-08 12:23:44	root[root] @ localhost [127.0.0.1]	59	0	Quit	

you can see,in database level,the second parameter of last query but two has quote ( (1, '2', 3) )

A related Github Pull-Request [GH-247] was opened
https://github.com/doctrine/common/pull/247

A related Github Pull-Request [GH-247] was closed
https://github.com/doctrine/common/pull/247

Ok this is much more complicated to allow then i thought. The problem is that the QueryComponents are passed by value, as an array, not by reference. That prevents changing them because this change wouldn't be visible in the output walker.

I can add a method to allow this in the OutputWalker for now, but generally this requires a bigger refactoring on the Query Components.

Added setQueryComponent() in SQL Walker to allow modification in output walker.

I'm afraid that this doesn't solve the initial problem at all. I'll try to describe it in more details to show what I mean. Suppose we have two doctrine extensions each of which contain its own tree walker. Each of these tree walkers need to modify AST and add new component to it (joined with some component already existing in the query). The first problem is that each tree walker has its own queryComponents array which is not passed between them, although they not necessary need to use queryComponents - they could use only AST. The second, bigger problem is that the Parser class does not know anything about modifications of queryComponents in tree walkers and cannot pass modified version to the OutputWalker. The goal of submitting this issue was to allow adding new components to the query in tree walkers which is not achievable by your fix. I think it may be the first step in the right direction. Maybe TreeWalkerAdapter should have public method getQueryComponents() which would be used by the Parser to pass modified queryComponents between different tree walkers and finally to the OutputWalker ? This would not break backward compatibility and solve this issue. What do you think about it?

I've tried to implement the solution mentioned in previous comment but it's also not so clean and easy as I thought. Each tree walker (including TreeWalkerChain) would have to implement getQueryComponents() and setQueryComponent($alias, array $component) methods. The same with SqlWalker, so the TreeWalker interface should have these methods, which would break BC in some way (walkers that do not inherit from SqlWalker or TreeWalkerAdapter will fail to compile). So maybe my first solution (PR #464) is not so bad for now? In the future queryComponents could be replaced by a special object or could be passed by a reference to allow modifications.

Marked as improvement as its not a bug.

A solution might probably implement an object holding all the QueryComponent, implementing ArrayAccess. So that way the state can be shared.

Just hit this while developing an ast walker... Will look into it too since I need it more than soon.

As a VERY UGLY workaround, I used a static variable and a custom sql walker in combination with my AST walker.

namespace Comcom\Versioning\ORM\Query;


use Doctrine\ORM\Query\SqlWalker;

class WorkaroundSqlWalker extends SqlWalker
{
    public function __construct($query, $parserResult, array $queryComponents)
    {
        parent::__construct($query, $parserResult, $queryComponents);

        foreach (VersionWalker::$additionalAliases as $alias => $value) {
            $this->setQueryComponent($alias, $value);
        }
    }
}

irrelevant

First, you are using internal API therefore you are on your own anyways.

This is marked as improvment now, the functionality works, it may just be inefficient.

Hi,

I'm marking issue as invalid because you're conceptually wrong.
What you're trying to do is telling that a collection of new entities is holded by a collection of Persistent entities.
The reference internally of PersistentCollection to ArrayCollection means a lot here.

Correct code would be you to regenerate the collection (a new ArrayCollection) and just assign it to setTournamentLocations($newCollection);

Does this explanation is enough for you?

Cheers,

Hi Guilherme,

If I do this:

$locations = new ArrayCollection();

foreach ($form->getValue('tournamentLocations') as $id) {
    $locations->add($em->getReference('Tournaments_Model_TournamentLocation', $id));
}

$bracket->setTournamentLocations($locations);

... then all the records are deleted, before adding the new records. This is inefficient and causes extra, unnecessary write operations.

Can't Doctrine perform diffs when persisting the collection, so that only the necessary deletes and inserts are executed?

We could add it, but I don't think it worth the effort.
Main problem with this one is that we use C-level binary comparison to get the diff. That's what you entities/hash pointers are different.
We would have to write our own diff-comparator for both collections, which would probably slowdown the entire Doctrine.

I'd rather consider that it's not possible to be done at the moment, but I need much more investigation for that. This will be something that I'll probably only do when I look at this issue again with a lot of time (which is really hard to happen).

If you have some spare time, feel free to make some attempts.
Just don't forget to enable performance tests in Doctrine Unit Test suite.

My pull request (https://github.com/doctrine/doctrine2/pull/548) contains an implementation of the method. Note that no exception is thrown if you query for the state of a non-existing filter - in such a case, false is returned as for disabled filters.

Whats the status here? Do we have any?

Since we moved to Symfony Console I don't think this is needed anymore.
The purpose of this ticket was actually to test our own CLI support, which was dropped.

I'm closing the ticket due to this. Reopen if you have any other comment.

I think we do need some basic functional tests of our Commands, they have been subject to many bugs in the past becaues they are not tested.

Fixed another fatal error in the command due to missing namespace dependency. We need tests for all the commands, there have been dozens of issues on these things so far.

This commit shows a simple approach on how testing is easily possible for symfony commands:

http://github.com/doctrine/doctrine2/commit/51e6681934a7cf4448b85c5670c04045f66c6056

Can we expect some more tests for beta4 or is it unlikely that you find the time? Should we move this further back or does someone else want to step in?

There is another approach for this using several subqueries to build an IN clause, the Paginator extension supports this: http://github.com/beberlei/DoctrineExtensions

I rather go the extension approach than changing the DQL for this feature.

I just saw your second example, that is rather cool though and gets +1 from me.

I had the same idea for "not initialized proxies", i.e.

$em->getUnitOfWork()->initializeProxies('Customer');

Second example is a duplicate of DDC-734

From reading the issue i know what the bug is, indeed this sucks.

@Jon: Any more information coming?

@Benjamin: Can you summarize the essence of the issue shortly?

@Roman: The UnitOfWork (may) still be pushed as a listener into that entity, and still recieve noticies of update. Which may throw notices because the oid hashes are removed everywhere. Additionally you cant serialize the thing because you still got the UoW inside there.

I don't have anymore information currently. The issue was relayed to me. I will try and find some more information and report back.

There is no way to "fix" this issue, i am turning it into a feature request. There needs to be a "postDetach" event that is triggered where the developer can detach the change notification objects.

Updating fix version

not a bug, its a feature (though a dumb one). Improvement in a next version.

Note: If the fist code block is always performed before the last 2 blocks then there is no issue here, just a need to make it more clear in Doc that this is possible but that you should not rely custom implementation as PersistentCollection will be injected when loaded from db.

When this will be fixed?

I think this is related to http://groups.google.com/group/doctrine-user/browse_thread/thread/7e1cfa9c4c99af31

Sounds logic, each driver would expect NULL or data (wrapped specifically for the driver used)

Requirements for batching of eager loads:

1. Since we are using an IN() query for this we can only support this feature for entities that have a single column primary key.
2. If we want to support composite keys we need to build it as WHERE ( (id1 = ? AND id2 = ?) OR (id1 = ? AND id2 = ?)) but this is currently not possible with the way how internally the $criteria array is used.

Next item to think about: What if an exception or event breaks the flow and "triggerEagerLoads()" is never called?

Descheduled to 2.x

Updating fix version

Updating fix version

Removed from master, as i dont like the api at all

Updating fix version

Updating fix version

Is there any fix for this? i have the same problem.

It makes a perfect sense here, I wish it was possible, it would give us a room for even more optimization. Any input on the issue from the developers?

Idea:

Develop a datastructure of sorts that allows saving information about skipping tables and columns therein when reverse engeneering.

This is not possible unless you take advantage of Topological Sorting to map class dependencies like we do inside of UnitOfWork AFTER creating the ClassMetadata.

The necessity of having this is mandatory because we can never skip classes that have associations to other ones though FK.
You may try that, but it doesn't compensate the effort. I'd rather mark this bug as won't fix, but I'm leaving for you do that. =)

Updating fix version

Has to be pushed back as immutable date time cannot be implemented in userland that well.

Updating fix version

Updating fix version

It occurs even if the value is not HIDDEN.

@Christian this sounds like a completly different error, please explain why you think this belongs here or open a new ticket.

Benjamin: Because I get exactly the same error message regardless of using HIDDEN. So HIDDEN doesn't seem to be liable here.

Can you paste your DQL and SQL?

Code:

$queryBuilder
	->select('myEntity, 123456789 AS distance')
	->where('distance <= 10')
;

DQL:

SELECT myEntity, 123456789 AS distance FROM MyCompany\MyBundle\Entity\MyEntity myEntity WHERE distance <= 10

SQL:

SELECT s0_.id AS id0, s0_.a AS a1, s0_.b AS b2, s0_.c AS c3, s0_.d AS d4, s0_.e AS e5, s0_.f AS f6, s0_.g AS g7, s0_.h AS h8, s0_.i AS i9, s0_.j AS j10, s0_.k AS k11, s0_.l AS l12, s0_.m AS m13, s0_.n AS n14, s0_.o AS o15, 123456789 AS sclr16, s0_.p AS p17 FROM myEntity s0_ WHERE sclr16 <= 10

That is expected behavior, ANSI SQL defines SELECT to be evaluated AFTER WHERE.

SELECT 1234 AS foo FROM `test` HAVING foo = 1234

DQL has a HAVING clause as well, not sure it works without the group by. Please try.

So we might just close this issue then?

Its not a bug, just the error message is supposed to be improved (if possible) in a cheap way.

Example:

$dql = "SELECT new CustomerAddressView(c.id, c.name, a.id, a.street, a.number, a.city, a.code)
             FROM Customer c INNER JOIN c.address a WHERE c.id = ?1";

This supersedes DDC-1819.

1. One additional property in ResultSetMapping => $viewModelClass?
2. Changes to Parser (new ... syntax)
3. Changes to sQL Walker?
4. Changes to Hydration (Only object hydration!)

Roman, I'd like to do this one as I have currently a use case for this. Do you have any idea of how to do this? What I'm wondering is whether it is possible to implement this without user intervention. (This would simply mean "store the entities as they were added"). But this would need another column in DB that we'd have to add within oneToMany / manyToMany relationships, but in this case one could save a serialized array holding "entityId => position" key / value pairs.

Afterwards, one could easily rebuild / reorder the collection via $collection->set($entity, $order[$entity->identifier]);

If you've got another thought about this, please don't hesitate to point me into the right direction!

this won't be implemented until 2.1, since its a pretty complex feature.

Changes are probably required in:

1. CollectionPersister - Add a new collection persister that takes the position into account
2. SchemaTool - Add a 'col_position' column to either the many-to-many or the one-to-many tables.
3. EntityPersister - Use and extend current order-by support to make the sorting happen

You can implement this already though with some performance hit in update scenarios. If you use the ORDER BY support and implement an API around your entity that abstracts those changes and always sets a "position" field on the many entity that is supposed to be sorted.

I don't think we necessarily need a new collection persister. Simply adjusting the ManyToManyPersister to be able to deal with it might be sufficient.

For OneToMany, that is always persisted from the "many" side, thus there is no collection persister, we would need to adjust the normal persisters.

They key element for the user should be a new annotation (or corresponding xml/yaml element) @OrderColumn. By default the order should not be persistent, only when an @OrderColumn annotation is present. The name of the order column can have a default, i.e. "position". Thus this enhancement of persisting the order should be fully backwards compatible.

On another note, the getInsertDiff/getDeleteDiff methods of PersistentCollection should already be "ready" for this. That is, when an element in the collection changed only its position, this is already tracked as a change. However the ManyToManyPersister issues no "UPDATE" queries, it simply deletes and inserts. A position change may be more effectively persisted with an UPDATE.

From a mailinglist entry, required check/changepoints:

1. ClassMetadata of Many-To-Many associations have to be extended to publish the required datastructure to the ORM.
2. All Metadata Mapping Drivers have to be extended
3. Persisters\ManyToManyCollectionPersister has to be extended to save the key in the many to many table if desired by the user.
4. Schema-Tool has to be extended to create the additional column.
5. PersistentCollection has to be extended so that lazy loading of collections with additional key works.
6. Array- and ObjectHydrator have to be extended to allow fetch join of collections with key column.
7. Discuss wheather to support this for One-To-Many also with the key-column on the many side. This is much more tricky internally though.

Push back to 2.x, we will have support for DDC-250 first and for this at a later release.

Hi there,
I'm looking for this feature.

Benjamin Eberlei said that : "You can implement this already", but I don't understand the "how to".

Also,
The problem should be solve if RDBMS had a "natural" order. An order based on item position inside table.

To get this feature without any change on Doctrine, I have remplace the PK defined by the target & mapped field identifier. The new PK is a new field with type "integer" and with auto-increment enable.
In this configuration, Doctrine use the "natural" order of the RDBMS. And I can change order of my item inside Collection and persist it.

It's an very bad solution, but It work before an official support.

Waiting for advices, and solutions,

Thomas.

Answering to Benjamin Eberlei on the "7. Discuss wheather to support this for One-To-Many also with the key-column on the many side. This is much more tricky internally though.".

I think that for One-To-Many relations, if user want to store the collection order, Doctrine can store the One-To-Many as Many-To-Many with a "model" limitation.
In that case, if storing order collection for Many-To-Many work, it should work for One-To-Many.

What do you think about it ?

I think that it must be possible to have two keys ordering : the order isn't obligatory reversible.

For exemple with user and group :

• You can order groups for one user : with preference by exemple, or importance.
• And with a different order, users for a group : rank by example.

And maybe more, if you decide to add multi-order : an user show group by his rank in it, if his rank is identical, the order is make by love preference, and after by the importance given by the user (not necessary a number, if we imagine filter on them).

So a default order can be choice with parametized fields and could be :

 
@ManyToMany(targetEntity="Group")
...
@JoinFields ( rank: { type: int} , preference:{type:int}, importance:{type: string, length: 40} )
@OrderByJoinFields({"rank" = "ASC", "preference"="ASC", "importance"="ASC" } )

In this case the order must be optional and would be clean if another order appears in the same scope (DQL...). And manytomany became virtual entities act as other entities except they don't appears permetting in the same time a better conception.

So if the solution take in DDC-181 will become the only solution. This would a good idea to document this. Because, this seems to me a very important point.

My last point is even an unique ordering field created in the join table will be a big and usefull improvement.

Thank a lot for your beautiful work.

In my point of view, a collection can be order in a single way only.
If you want to add more than one order between User & Group, it's a new collection, a new relation.
Like :

• User.memberOf() : Group[]
• Group.members() : User[]
• Group.importantMembers() : User[]

And it's your role to keep a consistency between members & importantMembers array.

Because ManyToMany join table is the reflection of a state of an ArrayCollection. It's not a usefull feature to be able to store all of the state of an ArrayCollection, even the order of this Array. It's just a normal feature that is really missing

Thomas.

I don't think:

If you have three collection, you duplicate one relation 3 times and it's easy in consequence to lost the data integrity and unicity.

By example :

• Thomas have rank 10 in Admin
• Thomas think the admin group has importance noted 3 on all of his groups.
• If a responsable of admin group decide to delete Thomas from it.
• Thomas, in his ordered list of groups, think always to be in group admin.

So in my idea, the many to many relation isn't just an array collection, but should be an virtual entity. In UML or in Merise method this is a common problem to have a parametized relation. I think an orm should just implement this.

Hum,
I agree with you.. In a SQL Schema, it's a good choice to add many fields in a ManyToMany join table to description "order".

I just want to add a piece of Doctrine ORM Document :

"When working with collections, keep in mind that a Collection is essentially an ordered map (just like a PHP array). That is why the remove operation accepts an index/key. removeElement is a separate method that has O ( n) complexity using array_search, where n is the size of the map."

Hi there,
After several discussions. on IRC, I have changed my point of view.

Doctrine Documentation says : "When working with collections, keep in mind that a Collection is essentially an ordered map (just like a PHP array)".
So, I think that Doctrine have to be able to store or not the order of a Collection. By adding a new field on the Joined table to store the position of each elements.

But I not agree with @Nicolas. Because in his case, he's talking about Association Class : http://etutorials.org/Programming/UML/Chapter+6.+Class+Diagrams+Advanced+Concepts/Association+Class/
Because he's talking of a business logic, he's talking of a dedicated Entity class.

What do you think about it ?

Thomas;

Any news ?

Hi, any news on this?

If I may add any info to this feature request, maybe something like JPA/Hibernate could be a good start?

See http://docs.oracle.com/javaee/6/api/javax/persistence/OrderColumn.html or http://docs.jboss.org/hibernate/core/3.6/reference/en-US/html/collections.html#collections-indexed

The idea in Hibernate is that you persist the order of the list in an extra column. This column is not a field of the entity however.

Questions

• I suppose link and unlinked entities would then handled by UnitOfwork commit also?
• Since the collection is not initialized, one does not know upfront if the action will be successful, what happens if:
  • an entity is linked with a collection, although they are already connected.
  • an entity is unlinked from a collection it is not in.

Regarding the naming, i like link/unlink.

What do you mean by "handled by UnitOfWork commit" ? Whether the SQL is "scheduled" or executed immediately? Interesting question.
Scheduling would probably be better but also more difficult.

As far as usage is concerned, I currently imagine it as follows:

// EntityManager#link($sourceObj, $field, $targetObj)
$user = $em->getReference($userId); // $userId probably from request parameters
$address = $em->getReference($addressId); // $addressId probably from request parameters
$em->link($user, 'addresses', $address);

"What happens if: an entity is linked with a collection, although they are already connected."

Probably an SQL error which results in an exception from the driver. Depends on the database constraints though.

"What happens if: an entity is unlinked from a collection it is not in"

Probably nothing, at least not from the SQL side. An exception could be thrown from Doctrine itself if the update affected 0 rows.

Thanks for these initial questions. Thats definitely food for thought. Keep it coming.

Pushed back.

If this is to be included in 2.0 it needs to happen for RC1. However, it is not clear yet whether it will be done in time.

How would you solve this Roman? I thought of adding a query hint so that the postLoad inside unit of work is not triggered and gathering all the entities that have a post load event in an array inside the object hydrator, then iterating it after taking the snapshots of all collections inside hydrateAll

@Benjamin: Not sure what you would use the query hint for but in general that is the approach I had in mind, yes. You can't get around iterating over the entities after the actual hydration.

The query hint would do something like:

        //TODO: These should be invoked later, after hydration, because associations may not yet be loaded here.
        if (isset($class->lifecycleCallbacks[Events::postLoad]) && !isset($hints['hydrationPostLoad'])) {
            $class->invokeLifecycleCallbacks(Events::postLoad, $entity);
        }
        if ($this->evm->hasListeners(Events::postLoad) && !isset($hints['hydrationPostLoad'])) {
            $this->evm->dispatchEvent(Events::postLoad, new LifecycleEventArgs($entity, $this->em));
        }

another way would be to move that code out of UoW::createEntity completly and have the persisters call it when they use that method.

Leaving that code in UoW does not make sense to me, if it is moved, it needs to be moved completely. Why do you think the persisters should do it? Initially I thought collecting the affected entities during hydration and then when hydration is done iterating over them and triggering the postLoad events.

Yes but postLoad has to be triggered for non Hydrated entities (i.e. Persister) also

Moved back

Hi Gyus, I need access to associations in postLoad or similar event, and my idea is to dispatch new event after full initialisation of object, what do You think about it? If I can help please let me know It's important for me.

Now in my PostLoad access to associations is work fine. Why this issue is still in Unresolved status?

What do you (Roman and Benjamin) think about adding postHydrate event which would be called within ObjectHydrator::hydrateAllData() on every entity collected during hydration? I could prepare a patch for this. I personally think this would be better than adding a hint that changes behaviour of postLoad event.

In this implementation Schema-Tool would generate a table:

elementTable (entity_id-1, ..., entity_id-n, key, value) and using the Platform Type Generation of keyType and valueType

Column Names should be Change-able also since there could be people who name their primary keys "key" and "value" o_O

Ordering could be implemented on top of this using the @OrderColumn JPA implementation by adding another column to the table with a numeric order that will be "order by"'d on select time.

Pushed back

@Roman A possible fix for this in my opinion is another map in UnitOfWork $mergedEntities = array(); and a patch like this:

diff --git a/lib/Doctrine/ORM/UnitOfWork.php b/lib/Doctrine/ORM/UnitOfWork.php
index 242d84b..1d0d8b3 100644
--- a/lib/Doctrine/ORM/UnitOfWork.php
+++ b/lib/Doctrine/ORM/UnitOfWork.php
@@ -1340,6 +1340,10 @@ class UnitOfWork implements PropertyChangedListener
             return; // Prevent infinite recursion
         }
 
+        if (isset($this->mergedEntities[$oid])) {
+            return $this->mergedEntities[$oid];
+        }
+
         $visited[$oid] = $entity; // mark visited
 
         $class = $this->em->getClassMetadata(get_class($entity));
@@ -1468,6 +1472,8 @@ class UnitOfWork implements PropertyChangedListener
 
         $this->cascadeMerge($entity, $managedCopy, $visited);
 
+        $this->mergedEntities[$oid] = $managedCopy;
+
         return $managedCopy;
     }

I have tested this patch with my application and it fixes the problem in all my relevant test cases apart from one. The test case that's failing is one that persists a bi-directional many to many relationship, so the associations interweave with each other (if you know what I mean).

I wonder if perhaps doMerge need to continue cascading even if it finds an item in $this->mergedEntities

This is the Flextrine code that fails - it results in no entries in movie_artist. This might also be related to DDC-758?

m1 = new Movie();
m1.title = "Movie 1";

m2 = new Movie();
m2.title = "Movie 2";

a1 = new Artist();
a1.name = "Artist 1";

a2 = new Artist();
a2.name = "Artist 2";

m1.artists.addItem(a1); a1.movies.addItem(m1);
m1.artists.addItem(a2); a2.movies.addItem(m1);

m2.artists.addItem(a1); a1.movies.addItem(m2);
m2.artists.addItem(a2); a2.movies.addItem(m2);

// These translate to cascade merges on the server
em.persist(m1);
em.persist(m2);
em.persist(a1);
em.persist(a2);

// Now flush
em.flush();

P.S. This test passes if I translate em.persist() to $em->persist() (not cascading) on the server instead of translating it to a cascade merge; not sure if that helps

I'd really like to avoid introducing an additional instance variable just to solve this issue but I did not find the time yet to really look into it.

Does someone have a unit test for this already and can attach it to the issue?

Rescheduling for RC1.

Here is a functional test case containing three tests:

testMultiMerge tests basic merging of two new entities, checking that only a single entity ends up in the database. This passes with Benjamin's patch.

testMultiCascadeMerge tests the more complex case of merging a OneToMany association. This also passes with Benjamin's patch.

testManyToManyPersistByReachability tests the ManyToMany case described above and this fails with Benjamin's patch, probably because doMerge doesn't cascade down entities that it has already merged and some ManyToMany associations are being ignored. Its a bit hard to be certain what is causing this as even without Benjamin's patch this test would fail due to DDC-758.

@Roman i thought about this issue, its not possible without that additional map of merged entities. There is no way we can get that information from other sources.

Problem is rather that the use-case probably only applies in mass-merging scenarios and client-server serialization.

Added another failing test case - adding the same entity from different ends of a many to many bi-directional association to check that there isn't an integrity constraint violation caused by Doctrine trying to add the same row twice.

Attached a patch for this issue.

can you comment why all the additionall stuff is necessary compared to my patch?

It fixes the two additional test cases - testManyToManyPersistByReachability and testManyToManyDuplicatePersistByReachability.

testManyToManyPersistByReachability was failing with your original patch because there are ManyToMany cases where an entity may have already been merged, but its still necessary to add it to an association and continue to cascade. Running the following with the original patch will miss out some of the associations.

$m1 = new Movie();
$m1->title = "Movie 1";

$m2 = new Movie();
$m2->title = "Movie 2";

$a1 = new Artist();
$a1->name = "Artist 1";

$a2 = new Artist();
$a2->name = "Artist 2";

$m1->artists->add($a1); $a1->movies->add($m1);
$m1->artists->add($a2); $a2->movies->add($m1);
$m2->artists->add($a1); $a1->movies->add($m2);
$m2->artists->add($a2); $a2->movies->add($m2);

$em->merge($a1);
$em->merge($a2);
$em->flush();

The other change in my patch is to protect against this case. It ensures that the following code doesn't add the same entity twice to a collection.

$em->merge($m1);
$em->merge($m2);
$em->merge($a2);
$em->merge($a2);
$em->flush();

I am not sure if the issue here is rather multiple calls to merge that contain different parts of the same object-graph.

There should be a very simple fix for this, call ->clear() after each merge.

I am not sure if this patch drags us into a blackhole of issues with merging.

Calling ->clear() and ->flush() after each merge is a workaround for the simple case, but unless I am misunderstanding I don't think its a solution for cases where the merging is happening automatically in cascadeMerge. I've actually encountered this issue in another project and scenario to do with creating REST APIs and merging JSON objects into entities, and applying the patch fixed it so a) I think this issue might be a more common that we first thought and b) the patch basically seems to work (plus it doesn't introduce any failing cases in the existing test suite). I can actually still find one edge case to do with cascading merging interlinked many to many associations that this doesn't fix, but I was planning to open that as a new ticket after this My feeling is that the current merge already has issues and this definitely improves it.

It cannot happen inside a single merge, single merges use the $visited to avoid infinite recursions, each entity can only be merged once inside a single merge operation.

Added a note into the documentation about using EntityManager#clear between merging of entities which share subgraphs and cascade merge.

Handling this issue in UnitOfwork will be declared an improvement, not a bug anymore and be scheduled for later releases. The required changes to the core are to dangerous and big.

Where in the docs is that?

Just to summarize, the equivalent operation to having multiple merges and a single flush is to call merge followed by flush each time, with the whole thing surrounded by a transaction? Does this have a big impact on performance?

Ben - even given the decision not to implement this (and I do understand your thinking, as it is a major change), is there any reason not to implement the bit that ensures that the same entity isn't added to a collection twice during a merge? I can't think of a situation where this should be allowed, and I have a use case where I get 'DUPLICATE KEY' errors if this isn't there.

Please see attached patch.

What bit of that huge patch is that? Can you extract it into another ticket if thats possible?

I added it to "Working with Objects" and the descripton of Merge. Its not yet live on the site.

Using this current workaround has a performance impact, since more SELECT statements have to be issued against the database.

Apologies for not being clear - only the 3rd patch (multipleaddmerge.diff) is relevant to the 'DUPLICATE KEY' error I am now talking about, but I'll put it in a nother ticket if you prefer.

please add a new ticket, patch looks good.

Created as DDC-875

This very minimal patch removes the use of these temporary files:

--- library/Doctrine/ORM/Proxy/ProxyFactory.php	(revision 2)
+++ library/Doctrine/ORM/Proxy/ProxyFactory.php	(working copy)
@@ -78,9 +78,8 @@
         $fqn = $this->_proxyNamespace . '\\' . $proxyClassName;
 
         if ($this->_autoGenerate && ! class_exists($fqn, false)) {
-            $fileName = $this->_proxyDir . DIRECTORY_SEPARATOR . $proxyClassName . '.php';
-            $this->_generateProxyClass($this->_em->getClassMetadata($className), $proxyClassName, $fileName, self::$_proxyClassTemplate);
-            require $fileName;
+            $file = $this->_generateProxyClass($this->_em->getClassMetadata($className), $proxyClassName, null, self::$_proxyClassTemplate);
+            eval('?>'.$file);
         }
 
         if ( ! $this->_em->getMetadataFactory()->hasMetadataFor($fqn)) {
@@ -144,6 +143,9 @@
 
         $file = str_replace($placeholders, $replacements, $file);
 
+        if ($fileName === null)
+            return $file;
+
         file_put_contents($fileName, $file);
     }
 

The proxy dir is used for the "doctrine orm:generate-proxies" command in the case of "autogenerate= false", so you need to define it anyways.

You have to use proxies, the option is not for Proxy yes/no. If you have autogenerate=false and doctrine requires a proxy for a use case but can't find it you will get a fatal error.

I just saw the eval() keyword, ieeks It could maybe be a convenience option for those that don't want to use proxy direcotiries, however i am not sure.

eval() is no different than writing code to a file and using require().

When using runtime-generated proxies, there are no benefits (that I know of) from writing them to a file. The disadvantages are:

• slower because of write disk access
• has problems with high concurrency, unless special care is taken (DDC-716)
• potentially has permission problems if code is executed by different users (e.g. nobody for a daemon and www-data for http)
• requires setup of a writable directory

This is a nicer patch, which makes _generateProxyClass() return a string, just like other _generate* methods:

--- library/Doctrine/ORM/Proxy/ProxyFactory.php	(revision 2)
+++ library/Doctrine/ORM/Proxy/ProxyFactory.php	(working copy)
@@ -78,9 +78,8 @@
         $fqn = $this->_proxyNamespace . '\\' . $proxyClassName;
 
         if ($this->_autoGenerate && ! class_exists($fqn, false)) {
-            $fileName = $this->_proxyDir . DIRECTORY_SEPARATOR . $proxyClassName . '.php';
-            $this->_generateProxyClass($this->_em->getClassMetadata($className), $proxyClassName, $fileName, self::$_proxyClassTemplate);
-            require $fileName;
+            $code = $this->_generateProxyClass($this->_em->getClassMetadata($className), $proxyClassName);
+            eval($code);
         }
 
         if ( ! $this->_em->getMetadataFactory()->hasMetadataFor($fqn)) {
@@ -107,19 +106,19 @@
         foreach ($classes as $class) {
             $proxyClassName = str_replace('\\', '', $class->name) . 'Proxy';
             $proxyFileName = $proxyDir . $proxyClassName . '.php';
-            $this->_generateProxyClass($class, $proxyClassName, $proxyFileName, self::$_proxyClassTemplate);
+            $code = $this->_generateProxyClass($class, $proxyClassName);
+            file_put_contents($proxyFileName, "<?php\n" . $code);
         }
     }
 
     /**
      * Generates a proxy class file.
      *
-     * @param $class
-     * @param $originalClassName
+     * @param ClassMetadata $class
      * @param $proxyClassName
-     * @param $file The path of the file to write to.
+     * @return string The code of the generated methods.
      */
-    private function _generateProxyClass($class, $proxyClassName, $fileName, $file)
+    private function _generateProxyClass(ClassMetadata $class, $proxyClassName)
     {
         $methods = $this->_generateMethods($class);
         $sleepImpl = $this->_generateSleep($class);
@@ -142,9 +141,9 @@
             $methods, $sleepImpl
         );
 
-        $file = str_replace($placeholders, $replacements, $file);
+        $file = str_replace($placeholders, $replacements, self::$_proxyClassTemplate);
 
-        file_put_contents($fileName, $file);
+        return $file;
     }
 
     /**
@@ -244,8 +243,7 @@
 
     /** Proxy class code template */
     private static $_proxyClassTemplate =
-'<?php
-
+'
 namespace <namespace>;
 
 /**

Scheduled usage of eval() for 2.1, if the following conditions exist:

1. Autogenerate is set to TRUE
2. No Proxy Directory is configured.

Great, this is even better. This way you can have 1) autogenerated in ram-only, 2) autogenerated in files and 3) pregenerated.

And the minimal amount of config needed to get up and running is reduced, which is always nice.

you should know though, eval is dead slow. It generates the necessary proxies on EACH request and that cannot be cached in APC.

It's no slower than current autogeneration (file_put_contents+require). TBH, I don't know why anyone would want to use that over eval(), but I don't mind it being there.

Pre-generation is, of course, a different thing. Seems like a valid tradeoff to offer: build/a bit of config/better perfomance vs. no build/no config/potentially slower.

Yes, that is because file_put_contents + require is a development only strategy. The manual clearly states that autogenerate has to be false in production.

Using eval() instead of producing and requiring the file in the case of enabled auto-generation of proxy classes sounds like a good improvement for 2.1 to make proxies more transparent during deveopment and for anyone for whom performance is no issue.

I'm increasing the priority as I think it is easy to implement for 2.1 and a good enhancement.

A note on why having the proxies written to a file can be useful even with autogenerate being on: it makes it really easy to check the proxy code being generated. I use that a lot currently.

The solution suggested, giving three possibilities is cool, though.

If this is to be implemented for 2.0, it needs to happen for RC1, therefore rescheduling to RC1. Feel free to reschedule to 2.x if necessary.

Only oracle supports lock timeouts and no other vendor seems to plan to support it. I move to 2.x, but i guess this would rather be an issue of user extension.

Ubuntu 10.04 + Oracle 11g Entreprise + PHP 5.3.2 + Doctrine2 Git (up-to-date)

This is the continuation of http://www.doctrine-project.org/jira/browse/DDC-616. Only the schema is different.

just for understanding this scenario:

Is this a One-To-One relation and the TABLE_TEST2 "inherits" the primary key from its parent TABLE_TEST1?

If yes, this construct is not yet supported by Doctrine 2, we still need to include an ID-Generator that supports this kind of schema.

Change for a more understandable use case. Note that it's not my real use case and that I work on legacy database on which I can't change the structure.

updated the issue topic to get a better grasp of what needs to be done here.

I have the same error with Mysql whit the same condition.

More details on the work to be done:

The relevant code is in Doctrine/ORM/Mapping/Driver/DatabaseDriver.php only.

The idea is currently many-to-many tables are detected by checking that the table has foreign keys on all the primary key columns (no additional columns!)

Now with the 2.1 feature of foreign key/primary key entities this is not necessarily true anymore. You can have the primary keys being foreign keys BUT have additional columns that are not part of the primary key. This has to be detected.

If a foreign key-primary-key entity is found that has additional columns a ClassMetadata has to be created and the associations have to be created with the "id" => true flag in mapManyToOne().

For what it's worth, I'm getting this error when I have a PK that is a single column and not a FK.

PRIMARY KEY (`id`),
UNIQUE KEY `cycle_station_id` (`cycle`,`station_id`),
KEY `station_id_idx` (`station_id`),
KEY `readings` (`readings`),
KEY `source` (`source`),
KEY `temperature_min_max` (`temperature_max`,`temperature_min`),
KEY `station_id_cycle` (`station_id`,`cycle`,`updated_at`),
CONSTRAINT `compiled_1_station_id_stations_id` FOREIGN KEY (`station_id`) REFERENCES `stations` (`id`),
CONSTRAINT `compiled_1_station_id_stations_id_1` FOREIGN KEY (`station_id`) REFERENCES `stations` (`id`) ON DELETE CASCADE
) ENGINE=InnoDB AUTO_INCREMENT=160833690 DEFAULT CHARSET=utf8 COLLATE=utf8_unicode_ci

Might need to be pushed back to a 2.0.x / 2.x.x bugfix release. Not clear yet.

Doctrine_Record defines a replace() method.

In the MySQL Doctrine implementation, however, it is not the same as INSERT .. ON DUPLICATE KEY UPDATE. The replace() method implemented in Doctrine_Connection_Mysql uses the REPLACE INTO syntax, which is a DELETE and then INSERT when the key exists. This is fine, except for tables that use auto-increment fields. The delete-then-insert operation yields a new auto-incremented value, whereas INSERT .. ON DUPLICTATE KEY UPDATE would not.

MySQL (and SQLite) REPLACE is a no go. It causes way too much disc I/O and worse yet totally screws up the on disk data structures because of the deleting.

Scheduled for 2.2

Evaluating this makes me sad, except MySQL support for this is rather non-existant, and the oracle merge is aiming at batch operations.

Should this be done with

1. Select first, then insert
2. Catch and evaluate exception then update

I am leaning towards 1.

Updating fix version