Done!

PHP 5.3, Ubuntu Lucid, MySQL 5, Apache 2.2

Looks like abug in the SQL Walker.

btw, you can use $e->getTraceAsString() to get a nice looking output for an exception. Its not as verbose as var_dump on the exception

No, it is a bug on ClassMetadata.

The var_dump on Association of parents refers to a NULL on joinTable.
Problem seems to be deeper. I am creating a test case, will figure it out soon.

Ok, it seems that on Mapping drivers we don't map jointables on opposite side.

So, if you map something on inversedBy and you grab the association from mappedBy side, you'll never have the jointable definition, because it is not exported to us.
This seems like a bug flaw we have on our code... not at the point of a critical, but we need to fix it asap.

That the jointable info is only on the owning side is by design, not a flaw.

Path to DDC-719 aswell as a possible issue with collection member implementation.

In http://github.com/doctrine/doctrine2/commit/35af98260a525a841c05be15f52f8df455000066 I committed a fix to this issue.
Should be working now =)

Reopening in order to correct the fixed version.

Closing with correct version.

Debian 5.0.5, Apache 2.2.9, PHP 5.3.2

The following fixes it on Linux, but I think will not work in Windows (iirc, rename() on Windows won't work if the dest file already exists, much less atomically):

--- Proxy/ProxyFactory.php	(revision 2)
+++ Proxy/ProxyFactory.php	(working copy)
@@ -144,7 +144,9 @@
 
         $file = str_replace($placeholders, $replacements, $file);
 
-        file_put_contents($fileName, $file);
+        $tmpFileName = $fileName.'-'.uniqid('', true);;
+        file_put_contents($tmpFileName, $file);
+        rename($tmpFileName, $fileName);
     }
 
     /**

I don't even know why this is needed. Can't the file just be returned as string and eval()'d, instead of being written to a file and then require()'d?

we should just change file_put_Contents into:

file_put_contents($fileName, $file, LOCK_EX);

Setting the LOCK_EX constant now, this should solve the issue.

However in high concurrency scenarios the "autoGenerateProxyClasses" flag should always be FALSE and the proxies be generated during build-time.

LOCK_EX doesn't fix it for me. Apparently it's still possible that the file doesn't exist:

E_WARNING (2): include(MyProxies/MyClassProxy.php) [<a href='function.include'>function.include</a>]: failed to open stream: No such file or directory

Please note that my above uniqid+rename suggestion only works if more_entropy is true, if you decide to add it.

However, I'd much prefer having an option of just not using these files at all. I've created an Improvement ticket DDC-717 for this.

i now underestand what you are doing wrong.

if you set autogenerate = false you have to call doctrine orm:generate-proxies

I'm not setting it to false!

> When doing concurrent requests with autogeneration of proxies enabled

ah ok, eval is just a workaround.

Closed again, See DDC-717 for a timetable of a fix using auto-generate = true in production.

Debian 5 (64bit), Postgresql 8.3, ZendServer 5.0.2, PHP 5.3.2, Doctrine-HEAD

This bug also leads to about 400 test failures in the Postgres ORM Testsuite

Fixed! Thanks for reporting.

Fixed

Fixed in master

This is another case for possible optimizations in my opinion, is it possible to access the parameter needles from the ResultSetMapping? If so then we should add a convertToParam method to each Doctrine\DBAL\Types\Type and allow conversions to take place or just do nothing. This woulld help with this issue, aswell as with other more complex types to be bound.

This affects the following loop inside Doctrine\ORM\Query:

http://github.com/doctrine/doctrine2/blob/master/lib/Doctrine/ORM/Query.php#L234

According to Roman this should already be possible with:

$qb->setParameter(1, new \DateTime('2012-12-21 23:59:59'), \Doctrine\DBAL\Types\Type::DATETIME);

can you verify this?

$qb->setParameter(1, new \DateTime('2012-12-21 23:59:59'), \Doctrine\DBAL\Types\Type::DATETIME);

No this featrue not work for me. (Same error message) I'm tested at latest BETA release & latest code from master branch (GitHub).

Fixed, QueryBuilder::setParameter() and QueryBuilder::setParameters() did not yet support setting parameter types.

PostgreSQL 8.4.3

Fixed

Fixed the UoW issue. Thanks!

As for the second issue, there is really only one way to skip the second database call:

Use a Fetch Join DQL to get both book and author and apply the pessimistic lock to that DQL. (Locking both the book and author)

All the other way already created the Author proxy object and you won't be able to specify a locking hint on those lazy loading select statements, requiring you to do the second locking call.

Hi Benjamin, thanks for the quick answer, I already thought you'd answer that. Only funny thing is that in my SQL log it seems the select query for the lock is called before the select query for populating the $author values! May be because it delays populating the values before its first use? In the lock could actually be done at once. Any ideas?

Ah ok that is a very good optimization case, let me think about how and if this is easily implementable.

I close this issue now and open up a new one

win7, apache2, php 5.3.2

This issue is a duplicate of DDC-656. which was fixed this weekend. The current master has a fix for it.

Okay, thank you for a quick response. Will take it into account.

the code inside the method that checks the production settings should simply use the getters instead of accessing the attributes array directly. So: $this->getAutoGenerateProxyClasses() instead of $this->_attributes[....].

The default values are encapsulated in the getter methods to avoid having a large array with default values that grows with each new setting even if the setting is never used.

fixed

Win7

Of course without "unknown macro" in the code!

this was fixed for the in() function already some time ago, good oversight

added

to description

fixed

php 5.3.2, ubuntu 10.4

fixed format

how is this a bug?

when you call $em->flush() values are inserted switched

Updating priority to critical

fixed

Postgresql

The problem seems to come from Doctrine\ORM\Persisters\SingleTablePersister::_getSelectConditionSQL()

I am also experiencing this exact issue. The value of the empty string in the 'IN' clause would appear to be the 'discriminatorValue' field on the instance of ClassMetadataInfo that gets passed into SingleTablePersister. For me, that value is null.

As a side note, this came up when I was trying to switch all my entities from using Annotation to XML. When using Annotations, I do not experience the problem with this query, which leads me to believe the issues lies somewhere in the XML drivers.

That's about as far as I've gotten trying to debug it.

Why do not juste change the _getSelectConditionSQL() to the following :

protected function _getSelectConditionSQL(array $criteria, $assoc = null)
    {
        $conditionSql = parent::_getSelectConditionSQL($criteria, $assoc);

        // Append discriminator condition
        if ($conditionSql) $conditionSql .= ' AND ';
        if (isset($this->_class->discriminatorValue)) {
            $values = array($this->_conn->quote($this->_class->discriminatorValue));
        }
        $discrValues = array_flip($this->_class->discriminatorMap);
        foreach ($this->_class->subClasses as $subclassName) {
            $values[] = $this->_conn->quote($discrValues[$subclassName]);
        }
        $conditionSql .= $this->_getSQLTableAlias($this->_class->name) . '.'
                . $this->_class->discriminatorColumn['name']
                . ' IN (' . implode(', ', $values) . ')';

        return $conditionSql;
    }

There is a fundamental flaw in your logic i believe.

The parent entity has to be part of the entity map, otherwise it cannot be used as a starting point for findAll() on the repository.

   <discriminator-map>
      <discriminator-mapping value="0" class="FirstClass" />
      <discriminator-mapping value="1" class="ThirdClass" />
      <discriminator-mapping value="2" class="SecondClass" />
    </discriminator-map>

But what if the FirstClass is abstract ?
Then it can't be part of the entity map, isn't it ?

However I should be able to select all its children classes. Anyway I think Doctrine should allow me to do it

even if it is abstract it is part of the hierachy and can be selected in DQL, repository and find methods. Since you cannot instantiate
the abstract class in userland anyways there is never going to be a problem. It still has to be done.

Maybe we should throw an exception in the mapping drivers if this is not done properly

Correct me if I'm wrong, but if I do such a thing then i'll have a discriminator value wich will never be used.
It seems strange to me but as long as it works

Thank you for the explanation.

add it for now, you are right its never used. We will look into a change.

Fixed in master

Fixed formating

Fixed: http://github.com/Shurakai/doctrine2/tree/DDC-647

Hey Christian, please don't mark bugs as fixed that have not been merged into the main repository yet.

Merged now

fixed.

PHP 5.3.2

fixed formating

Fixed

Fixed in http://github.com/doctrine/doctrine2/commit/69073c4b37ee28f988306db4965f512b70f45181

mysql Ver 14.14 Distrib 5.1.43, for Win32 (ia32)

PHP 5.3.1 (cli) (built: Feb 8 2010 22:11:49)
Copyright (c) 1997-2009 The PHP Group
Zend Engine v2.3.0, Copyright (c) 1998-2009 Zend Technologies

• with Zend Debugger v5.3, Copyright (c) 1999-2010, by Zend Technologies [loaded] [licensed] [enabled]

Fixed and scheduled for BETA 3

Hi, I've got this exact issue in 2.1.0BETA1. Any idea how to work around it?

This is most probably equivalent with the following problem:

It's got to do with cli: when you DB is up to date and you run orm:schema-tool:update --dump-sql, you get something like this:

ALTER TABLE user_users CHANGE avatar avatar LONGTEXT NOT NULL <- but the column avatar is already LONGTEXT

This is caused by the following: Two schema objects are being compared. The first schema is created from the current mapping files (YAML, e.g.) and within these, this column is marked as "object". When the DB is setup, object is transferred into another type, like LONGTEXT. (Platform dependent)

The second schema is created from your current database and is marked as "TextType" because Doctrine cannot decide which other type - there are several who use LONGTEXT - would be correct

See /DBAL/Schema/Comparator.php ll. 172ff. & 293

Comparing whether both Types return the same SQL-Statement might do the job but would be really ugly.

The columnDefintion affected field can obviously not be solved by any means.

The foreign key thing i see more often in my own projects though, i have to dig deeper there.

Fixed Foreign Key issue in DBAL master, it was an error in the MySQL query to fetch the foreign keys.

I think this issue is a duplicate.

Was fixed by guilherme, see DDC-614

Test case

INDEX BY got optimized away, the SqlWalker currently does not interpret the INDEX BY clause.

Fixed

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

Fixed and scheduled for BETA 3

Something is broken with your commit:

Fatal error: Uncaught exception 'Doctrine\ORM\Mapping\MappingException' with message 'No identifier/primary key specified for Entity 'OUTILLAGE_OPERATION_COLLECTE'. Every Entity must have an identifier/primary key.' in /mkk01/doctrine/doctrine-orm/lib/Doctrine/ORM/Mapping/MappingException.php on line 37

Doctrine\ORM\Mapping\MappingException: No identifier/primary key specified for Entity 'OUTILLAGE_OPERATION_COLLECTE'. Every Entity must have an identifier/primary key. in /mkk01/doctrine/doctrine-orm/lib/Doctrine/ORM/Mapping/MappingException.php on line 37

In file 'lib/Doctrine/ORM/Mapping/Driver/DatabaseDriver.php ' (http://github.com/doctrine/doctrine2/commit/b7db8df7efed4517859be562fd58e6ce4cc6354a) around line 86, the iteration is not complete and it is not able to define the primary key

It seems the detection of many to many tables can cause severe problems. Some internal refactoring has to be done to get this reverse-engineering right.

This should now be finally solved. Many-To-Many tables are detected and supported in reverse engineering now.

PostgreSql 8.4

PDOException

SQLSTATE[42P01]: Undefined table: 7 ERROR: missing FROM-clause entry for table "t2_" LINE 1: ...N terminal_file t1_ ON f0_.id = t1_.file_ref AND (t2_.id IN ... ^'

Join

                        $qb->innerJoin('f.terminals', 'cts', Expr\Join::WITH,
                                $qb->expr()->in('cts.terminalRef.id', array_map(function($value) {
                                    return (integer) $value;
                                }, (array) $value)));

DQL

SELECT f FROM [...]\File f INNER JOIN f.terminals cts WITH cts.terminalRef.id IN(5) [...]

SQL

SELECT [...] FROM file f0_ INNER JOIN terminal_file t1_ ON f0_.id = t1_.file_ref AND (t2_.id IN (5)) INNER JOIN terminal t2_ ON t1_.terminal_ref = t2_.id [...]

On http://github.com/doctrine/doctrine2/commit/2c28872af820a27b36e4ff3ca28ef92ea8c1f0f3 this issue was fixed.

Fixed, throwing an exception now

Note: Not all child classes must be specified, only those that are entities. You can have non-mapped as well as mapped-superclasses as subclasses as well.

The validation task to perform here would be:

Throw a warning if there exists and entity that extends the Parentclass, is however not part of the DiscriminatorMap

Implemented

Fixed.

This should be changed for BETA3.

Fixed and scheduled for 2.0 BETA 3

Actually patch was only committed now.

http://github.com/doctrine/doctrine2/commit/5719f8523bbb3c8a6fd11c749e5317b238d3d2b3

Is this issue still valid? Is Doctrine Common Beta3 still affected? or is it an ORM issue?

Cannot reproduce, i have added a test-case that shows exactly the described code:

http://github.com/doctrine/common/commit/3b42776ada70cf4161082c155f5d8ba9326ce674

However here the relevant annotation is detected correctly in both cases.

Closed.

ubuntu x86_64

PHP Version 5.3.2-0.dotdeb.2 (/etc/apt/sources.list: deb http://php53.dotdeb.org stable all)

Zend Engine v2.3.0, Copyright (c) 1998-2010 Zend Technologies
with Xdebug v2.0.5, Copyright (c) 2002-2008, by Derick Rethans
with Suhosin v0.9.31, Copyright (c) 2007-2010, by SektionEins GmbH

I dont understand this issue, can you please elaborate what exactly is wrong?

To be more verbose:

I have two tables article and category.
These tables are connected via a m:n-relationship with table article_category.

You can see the entity configuration snippets within the code Entities.

To reconstruct this bug, set up your database with these 3 tables. Put in some data, but keep the table article_category empty.

Now, with an properly configured entity manager, you can execute the displayed code.

There is a line

printf("-- %s\n", count($article->getCategories()));

Please adjust array(4) to use valid category primary keys.

You will notice, as a result of this script, that sequential uses will produce

-- 0
-- n
-- 0
-- n
-- 0
... 

with n the number of categories you add. it will be 1 in this example (see: array(4))

If you check the database between the runs, you will notice that the table article_categories is filled, empty, filled, empty, filled, ... corresponding to the script results.

SOLUTION: You can solve this with one extra line:

$em->flush(); // [*]

I think the described behavior is not intended.

The problem is that clear() is actually scheduling the deletion of the whole colleciton. This is a completly different operation compared to:

foreach ($col AS $obj) {
    $col->removeElement($obj);
}

If you do it this way and than re-add some of the objects, those associations will not be changed.

clear() however deletes all, then adds the new ones. again

Verified as a bug though.

I see.

I suggest an additional method:

public function removeElements() { ... }

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

Not necessary, this was actually a bug. It should be fixed in the current master.

To match our naming conventions that should rather be: interface EntityRepository and the old EntityRepository => BasicEntityRepository or similar.

Roman, do you mean that we should rename the file (and class) EntityRepository to BasicEntityRepository and create a new EntityRepository that contains an interface EntityRepository?

If you want me to, I could do this.

Regards
Christian

@Christian: Yes, thats what I meant. I will schedule this for BETA3 though. I want BETA2 to be a smooth upgrade without any BC issues, if possible.

I am wondering whether this is really necessary? There are not many methods on EntityRepository and its easy to create your own interface(s) for your repositories that include these few methods. To be consistent with our naming standards there would have to be a bc break which seems unnecessary to me, unless we invent some other name for the interface, i.e. "ObjectRepository".

Wont fix

It's unfortunate this wasn't implemented, it's actually a significant issue for us. I have to say I completely disagree with Roman's reasoning in his last comment.

We have a very large database and commensurately many repositories. We're now at a point that we need to fire our own application level events (as an example, we end up having to compose in services a lot), some of them originating within repositories (doctrine events are insufficient, and we need application specific ones). We're using Symfony2 for DIC and build repositories not through doctrine but via services. We consider using setter injection to be a significant anti-pattern, an object should be valid/fully-initialized post construction so we'd rather not do that – if you were wondering. Now for all our repositories that we would like to build in extra functionality we create our own repository class, compose in the doctrine repository and implement the interface by convention (we could write our own, but the mileage sucks). Now where we could have passed in an instance of anything obeying that interface we're stuck (there goes type hinting). We can avoid some drudgery through traits but it's still an unfortunate solution.

If this is at all possible (even if it has a less than ideal name), we'd very much appreciate this, thank you.

PS. A BC break in beta would have been very easy and now it sucks even more.

Saem Ghani there is such an interface in doctrine common. Check Doctrine\Common\Persistence\ObjectRepository at https://github.com/doctrine/common/blob/2.3.2/lib/Doctrine/Common/Persistence/ObjectRepository.php

Marco: I'm afraid that is incomplete. EntityRepository has a number of methods that fall outside of the scope of those prototyped in ObjectRepository and Selectable interfaces that it implements.

Why don't you ship your own layer of repositories and disregard the Doctrine ones completly? I do that all the time, and then constructor injection is very simple as well.

Because the repositories provide features we use. Reusing/staying close to Symfony/Doctrine makes it easier to learn/train, and gives us herd immunity benefits around shared knowledge and most importantly testing. We're trying to reduce maintenance burden, this interface would not only help us do that but anyone else in a similar situation. Additionally, we have many projects most of them large, going across them we've got something like 400+ tables.

Saem Ghani your last comment does not really provide any rationale behind extraction of the remaining methods into an interface. There's no advantage in extracting those methods to an own interface. createQueryBuilder, createResultSetMappingBuilder, createNamedQuery, createNativeNamedQuery, clear, __call, getEntityManager, getClassMetadata are all utility methods that are not good candidates for an interface. You can easily re-implement those on an existing entity manager.

Just use the Doctrine\Common\Persistence\ObjectRepository API: you should actually stick to that to increase portability across the various object managers in doctrine project.

If you need all those methods too, you are looking for inheritance, not composition (and probably are delegating too much responsibility to the repository).

Marco, rather than looking at each comment in isolation (which is what seems to be happening), please take things in aggregate. I've provided the rationale in previous comments. If you require more information I can clarify further.

Extracting as an interface would:

• Make it easier to make mocks during testing (right now a mock EntityManager and ClassMetadata instance must also be mocked out), this is significant friction/noise during testing
• Allow people to compose as opposed to inherit, especially important when you have features that are specific to relational stores
• We have Entities that are represented by data in the database AND on the file system. All of a sudden inheriting from a doctrine EntityRepository makes just as much sense as inheriting from a FileSystemRepository (of our own making). All the while the responsibility is the same, load/store of Entities, the finders/query methods still make sense.
• At the same time the finder API is very useful (duplicating the code would suck, as was suggested earlier), but having to manually maintain that contract with each doctrine release is an unnecessary burden, not just for us, but anyone else in our predicament
• Creating our own interface means we can't abstract over 3rd party repositories provided by other bundles.

I think the order of operations in your example is not correct even though the error is misleading.

You are associating a "new doctor" to a "detached patient". That does not seem right, remember that merge() returns a managed copy, thus when merging the patient later, the "new doctor" is still associated with the "detached patient", not with the managed one.

The following should work:

// Merge detached patient
$managedPatient = $em->merge($patient);

// Associate new doctor with patient
$doctor->patients->add($managedPatient);
$managedPatient->doctor = $doctor;

// Persist new doctor and flush
$em->persist($doctor);
$em->flush();

You are quite right - that does work.

However, I am now trying to implement my use case (turning a tree of detached objects into a tree of managed objects) and implementing the correct order you describe above seems to cause another problem. I am not sure if this should be another ticket, but I'll put it here for the moment.

Note that the part within the stars that creates the unmanaged doctor and the detached patient simulates exactly what is received by Doctrine in my application so I can't put any merges in here.

/********************************************************************/
// This is a new element
$doctor = new \vo\Doctor(); $doctor->name = "New doctor";

// This is a detached element which is in the database
$patient = new \vo\Patient(); $patient->name = "Existing patient"; $patient->id = 1;

$doctor->patients->add($patient); $patient->doctor = $doctor;
/********************************************************************/

// Now replace $patient with its managed version
$managedPatient = $em->merge($patient);
$doctor->patients->set(0, $managedPatient);

// Persist the doctor
$em->persist($doctor);

$em->flush();

This works up to the flush, which throws an error of the form:

Notice: Undefined index: 000000007dd346c3000000005d0908d2 in \Doctrine\ORM\UnitOfWork.php on line 1955

In the database this results in a new doctor being created, but doctor_id in the patients table is set to NULL for the patient that is supposed to be linking to it.

I think this can't work, because obviously $doctor->patients still points to the unmanaged $patient, not the managed one.

Can you elaborate on why the star part is done by Doctrine? Where is doctrine doing that? what are you doing with the public API?

All the detached entities should be merged before doing anything regarding a NEW entity in my opinion.

Moved to beta3 for now

Patch with test-case for this merging scenario

Fixed in http://github.com/doctrine/doctrine2/commit/69073c4b37ee28f988306db4965f512b70f45181

PHP-Version 5.3.2
PHPUnit-Version 3.4.11
PDO Driver for MySQL, client library version => 5.0.83
doctrine-trunk@7546
Kubuntu 9.10 / 32bit

Forgot to tell trunk revision

This issue is mainly caused by the entity not being initialized before serialization. Additionally, the PersistenCollection does loose all information that is needed to regain the kept entities because the collection itself is not initialized before serialization.

I've added an initialization call here http://github.com/Shurakai/doctrine2/commit/6c185a2891111dfbd83d381bad8c5a2b16536cad#diff-0

However, I'm not sure whether this is the best solution. Any thoughts?

@Christian: I looked at the testcase you committed there and the assumptions it makes are not correct. The original test case provided by Markus made the right assumptions, that is, after the user is unserialized it can and should not know about its groups or phonenumbers since these were not serialized.
When you serialize an entity, you serialize a partial snapshot of its state. When you unserialize it you have all the state that was loaded prior to serialization but you can not get at the rest, the object is detached from the rest of the object graph. Thats where merge() comes in, it reattaches a detached entity to a managed environment where associations can be lazy-loaded, state changes are tracked, etc.

So the problem must come later, at the point of merging.

Pushing back to beta3.

Fixed.

The issue you have is the same as if you use result cache. The entity is not managed by EM.
Maybe an "Entity of class " . get_class($entity) . " not managed by EntityManager." exception is the best solution here.

Cheers,

Given that the combination:

$hash = spl_object_hash($object);
return $this->somefoo[$hash];

is probably one of the most called constructs in the complete code I tend to disagree with a check on each and everyone of them. However i to see the tendency towards errors of this kind as annoying, but maybe we can catch them earlier in those spots where they occour most often?

I think we dont use a read - update - write approach that is supposed to be atomic anywhere. Its obvious that this is fragile. Its like the select max() + increment in memory + write record variant for generating primary keys which is obviously a bad idea (at least it would need SERIALIZABLE transaction isolation I think).

(Obviously, any object retrieval -> modification -> persistence is a read-update-write but thats expectedly not "concurrency safe". Thats what optimistic or pessimistic (not yet implemented) locking is for.)

As this is a "reminder" and I dont see any issues currently, this is surely not a blocker until someone finds a serious (and valid) problem related to this.

and i want to reverse that logic .. i want to ensure that things have been reviewed so that it is clear that things will work concurrently. for example using CURRENT_TIMESTAMP, even just as an option, for versionable versions is uhm a very bad idea. at least make it microtime.

also D2 needs to support LOCK TABLE and it needs to be ensured that its possible to write "behaviors" that figure out the list of tables that need to be locked at the beginning of a flush() execution, because without it you are in a world of concurrency hurt when you want to implement stuff like Sortable or NestedSet on top of D2.

What is your definition of "that things work concurrently" ? The default transaction isolation for most databases (READ COMMITTED) is designed to allow concurrency for more liveness. This implies that things like lost updates and other stuff are allowed to happen. If you dont want that you can simply use a SERIALIZABLE isolation level for the transactions which will give stronger consistency but less liveness/performance, right?

I'm not convinced (yet) that timestamps are a "very bad idea" for optimistic locking (thats what it is used for, there is no "versionable" behavior or whatever built into Doctrine). Yes, you can still get lost updates in rare, high concurrency situations but most of the time lost updates are prevented and if this is fine for the particular application and a timestamp is a more useable and meaningful value then I dont see anything wrong with it. We added that feature because it was in the JPA spec and every major ORM I know supports timestamps for optimistic locking and I'm relatively confident that they know what they're doing (Its still possible that we just have a broken implementation in Doctrine, I dont know that yet, I didnt have the time to look in detail at the other implementations).

As for LOCK TABLE, we will probably provide an abstraction over the specific SQL in the different platforms in the DBAL. Then maybe Doctrine will use these in some scenarios for pessimistic locking but that is stuff for the future.

I'm not sure where you're aiming at with this ticket. Yes, locking features are going to be improved. Yes, concurrent transactions can cause lost updates and other stuff unless you use a higher isolation level. But this is all not new. The transaction isolation level is already under your control. The locking features will be improved. Where is the flaw you're apparently seeing? At least I get the feeling out of your ticket and comment that you seem to be very worried about concurrency, thinking that Doctrine doesnt do enough or does it wrong. I, however, dont yet see what more we should do than to provide support for optimistic and pessimistic locking and I dont see the world of concurrency hurt you're seeing and in which way this is the fault of Doctrine. I think I am very aware of concurrency-related problems but as its often a very difficult topic, its easy to overlook something.

i am aiming at being able to deliver algorithms on top of D2 that ensure the C in ACID .. for simple INSERT/UPDATE etc you can rely on transactions and native isolation levels, but for the things D2 needs to be able to do (versionable, sortable etc.) we need more than that. of course most apps do not going to have a bazillion of parallel requests .. but its still quite common for people to double submit .. or for multiple admins spotting and fixing the same data issue at the same time and if this breaks your nested set tree .. you are screwed. currently i have no reason to believe that D2 was written with sufficient brain cycles spend on these issues. if you are willing to release D2 1.0 without this being checked, then i think you are doing a horrible mistake.

D1 behaviors have tons of flaws that you have mentioned in your blog. but the biggest one never got mentioned, nor did it ever generate a response when i mailed the dev list. i fear that there is just too little attention being paid to this topic, maybe also because there isnt enough expertise in this area. but i think its mainly just not being looked at enough.

anyways, if you are certain enough that none of my concerns above are valid and that if there are any issues you can fix them without a BC break, then close the ticket.

"i am aiming at being able to deliver algorithms on top of D2 that ensure the C in ACID .. for simple INSERT/UPDATE etc you can rely on transactions and native isolation levels, but for the things D2 needs to be able to do (versionable, sortable etc.) we need more than that."

I assume you mean the "application level" consistency right? Such that an ordering is not messed up with 2 items having the same position or that a nested set tree is not corrupt? I understand that but why can't these be solved with a higher isolation level for the operations that modify the order/the tree?. I would be especially interested in such examples that can not be solved by using a higher isolation level. I currently cant see how it would be possible to break consistency of a sorted association or nestedset tree when using a SERIALIZABLE isolation level for the transactions that modify the order or the tree.

I'm not trying to dismiss your critiques here, I'm taking you very seriously, but you need to get more specific than "ensure the sql works with concurrent requests". That alone doesnt tell us much. We need concrete examples of problematic concurrent scenarios and what Doctrine itself can or should do to avoid these problems.

Currently, we dont know what to look for.

Thanks for your help.

the issue is that with FOR UPDATE .. and ultra paranoid isolation levels you can get locks on things that exist, but its not possible to necessarily get a LOCK on things that are just being inserted or altered to suddenly fall in your "range" or worse yet that get appended to your "range". some RDBMS do support range locks (like if I do WHERE foo BETWEEN 1 AND 100), but not all do .. and IIRC you do not really have much control over those (its simply a LOCK escalation strategy RDBMS may use to reduce the number of row level locks). this means in many situations fetching data with a FOR UPDATE even, might not be enough to ensure consistency when you do rights later on.

i have not done a code review of D2 .. actually i have spend very little time looking at any of the code, but from the questions and comments i have seen, there doesnt seem to be enough emphasis. for example i also saw code for the slug extension. which again employs the model of fetching all potential collisions and then doing an insert on the fetched data, without ensuring that between fetching and inserting there are no new collisions. stuff like this should try harder to do things in a single commit if possible, or getting the necessary locks.

locking is non trivial business and so expecting end users to get this right is a bad move imho.

but aside from any of the internal sql, i would also urge to ensure that some of the things i have been asking for (an efficient way to determine the affected tables and if they need to be locked at the start of a flush) a solid way to mark specific models or all model instances as dirty to ensure that users can either choose to reload certain dirty models at once or automatically when they are accessed again (see DDC-343) is really possible.

at the same time i acknowledge that i am just pointing at theoretical problems, without a promise to do the actual analysis and fixing. its just that this month i have promised people to spend my spare time on other stuff. next month might be more doable, i am traveling a lot .. where i can hopefully spend some time reviewing, then again traveling means that providing feedback and discussing things is more difficult.

Ok I have read some stuff, InnoDb for example locks on index ranges also locking the prev and next keys to avoid insert behind problms.

We are also planing to add support for "FOR UPDATE" and "FOR SHARE" as a query hint to all DQL queries aswell as a "Lock" Enum that allows to set this flags for transactions with the Entity Repositories (i.e ntityManager::find(), EntityRepository::findOne, findall() and such). See the linked issue DDC-178.

An automatic lock table is slightly more complex to compute but its possible using the "onFlush" event. My draft of a table locking manager is attached to the ticket.

A TableLocking Manager, usage:

$lock = new TableLocking();

$evm = new EventManager();
$evm->addListener($lock);

$em = EntityManager::create(..., $evm);

// do stuff
$em->flush();
$lock->unlockTables();

There should probably be a postCommit() event or something to clean up the mess automatically

Btw, this won't work at all with Joined Inheritance.

@lsmith: can you comment on the code?

This is looking quite promising. I guess it would be wise to give each model class the choice if it wants to cause a lock or not, because especially in high concurrency scenarios locking a table can of course have some drastic performance effects. Actually if I for example have a sortable behavior and I am not messing with the sorting at all, I do not need to lock the table either. So probably the best approach would be that if a model method is called that requires locking. Each model could register an onFlush() event (theoretically it could try and be smart and only register the event when it knows it has something to do) and then given the value of the flag tell the lock class to issue a lock for the given underlying table.

BTW: as for innodb .. like i said some RDBMS have this behavior .. but its nothing we can rely on at all ..

yeah of course, this code locks all the time, however some kind of configuration to check for models that need full table locks is probably necessary, but as you can see from the code, easy to implement.

My idea would be to add a "TableLockRequired" marker interface and in the loops check if any dirty model implements this marker interface.

Just as a reminder, I think if some code attempts to lock and the current RDBMS/Engine doesn't allow the requested lock, D2 should (make it an option maybe, but default true imo) throw an exception saying locking doesn't work with the current engine. Silently doing nothing is good in production to avoid pages exploding, but in development I want to know when I lock if it's not really locking in the background. And if I can know that without reading all detailed specs about whatever engine is in use that's even better.

Synchronizing schedule with DDC-178. This issue is considered resolved when the improved locking support from DDC-178 has been implemented. As Benjamin demonstrated more custom (and generic, reusable) locking strategies are possible through the use of the event system.

resolved.

Cli tests with PHPUnit
PHP 5.3.1 (cli) (built: Nov 27 2009 10:08:49)
PHPUnit 3.4.1

Patch tentative. Resolved this bug but breaks another test of DetachedEntityTest, so I added another to make clear what is the issue: relying on keys to know if an entity is detached is not correct when entities have natural keys like CmsPhonenumber.
It seems that the only option would be hitting the database to see if the entity is already present.
Eliminating the error and simply persisting the entity anyway would not be cool because a duplicate will be created everytime entity is passed to persist(); even to pass it internally to merge() we have to know if it's new or detached.
A last option would be declaring: pass detached entities to persist at your own risk. It's normal for Doctrine not being aware of the difference between new and detached, if not 'detached' would not be so detached if the Orm remembers it. I have no problem in keeping new entities separated from detached ones in my data structure. Or I can simply merge() all, catching exceptions and add the entity that resulted in exceptions with persist().

Yes, that exactly is the problem and I am/was aware of this but hitting the database is a no-go for that. Determining the state (UnitOfWork#getEntityState) has to be very fast. So its just a matter of finding the best compromise.

We can't save any information in Doctrine objects because serialized or cached entities travel between http requests and sessions. merge() uses the same assumption that entities without identifiers defined are new and when passing a phonenumber it gives a false protection because the defined natural key does not result always in a real entity in the database.
Substituting a proxy subclass which adds a $_state private property is definitely an overkill. I suggest removing the exception and assuming that detached objects should be passed to merge() before any operation is performed on them, and only detached objects should be passed to merge(). Behavior if other operations are done previously would be undefined.
The use case for the merge()/detach() methods is serialization/unserialization of objects from a cache. A developer would then only merge objects in the access point of the cache or on saving.

My take at this issue:

1. _doPersist() assumes all entities are new. This is wrong.
2. An entity is new if it has no identifier.
3. Detached entities have identifiers.

Now the heuristics:

1. If the IdGenerator is NOT (Assigned <-> None) and we find identifiers then it is obviusly a detached Entity
2. If we dont find identifiers the entity is obviusly is new.

Problematic case: IdGenerator is Assigned and we find identifiers: Here i assume the entity is new, by definition of an entity persist() leads to a duplicate key error and catches this error.

I think this is the best approach.

Updated version that introduces the "EntityExistsException"

The problem I have with this is that by doing this, every single entity passed to persist() gets its identifier read out using reflection just to determine whether its really new.

I would rather change the contract and the documentation to clearly indicate that detached instances must not be passed to persist().

There are no problems in a stricter contract for persist() if detached instances are kept separate from new ones in applications, which is usually true since they come from a cache or a store. I would go with the change as it's the simplest reliable solution.

I updated the documentation to be more clear about this issue: http://www.doctrine-project.org/documentation/manual/2_0/en/working-with-objects:persisting-entities

What do you think?

The problem is, we can not do it reliably for all cases. Yes, we could look into the identifier property if the identifier is generated and conclude from that whether it is detached or not but that does not work for manually assigned identifiers. For these we would need to hit the database and I just don't think we can do that.

So right now changing the persist contract as I did in the documentation, explicitly stating that detached entities should not be passed to persist() and that the behavior is undefined if done so, is the "best" thing.

Tell me what you think. The alternative would be to do the above (peek into identifier field of generated identifiers or look the identifier up in the db if it is assigned manually). I'm pretty sure the JPA implementations do just that but I dont know for sure and I dont think we want that.

I am still undecided though Something inside me prefers to go the other route, even if it means some additional db hits for manually assigned identifiers. Please make your vote.

If an identifier is a natural key, and not generated, a detached object of that class and a new one are really indistinguishable if they have the same values on the other columns.
I am for simplicity: the detached/merge entities are related to use cases like caches, sessions and serialization of every kind. So typically you in an application you always know when they come from such a source and can call merge() appropriately.
If calling persist() erroneously will result in throwing an exception on flush() (primary key value already present), the application developer will notice the problem.

Pushing final resolution back to beta3. The problem is that this missing detection of detached objects can lead to weird errors in other cases as well. Need to think about it some more.

We should be aware that PHP 5.3 now uses mysqlnd driver internally.
This means that queries like LOCK are applied via unbuffered queries, which may compromise subsequent data changes on DB table.

We need to do some testing before effectively apply any type of approach here.

What does this have to do with locking? I don't understand it Please elaborate

Attached a quick try for pessimistic lock.

Optimistic lock enforcement is much more difficult...

Update of the patch adding tests to the Query SELECT tests.

Regarding Nr. 5, indeed more than one root entity is supported in DOctrine 2 when querying.

Example (assuming Customer and Employee are not related in any way):

$q = $em->createQuery("SELECT c, e FROM Customer c, Employee e WHERE c.hatsize = e.shoesize"

The result would (or should) be an array with both Customer and Employee objects.

We should probably go only for the PESSIMISTIC lock in the interface.

However it might be convenient to make optimistic locking possible via an additional parameter. Given that PHP is Stateless it might be a good pattern to do:

$id = $_POST['id'];
$lastKnownVersion = $_POST['version'];
$entity = $em->find('Yadda', $id, $lastKnownVersion);
// do stuff with $entity

$em->flush();

Hm given our discussion yesterday, we should translate the OPTIMISTIC_FORCE_INCREMENT into PHPs execution context (script per request):

final abstract class Lock
{
    const NONE = 0;
    const OPTIMISTIC_READ = 1;
    const PESSIMISTIC_READ = 2;
    const PESSIMISTIC_WRITE = 4;
}

OPTIMISTIC_READ requires the fourth parameter of EM::find() to find being a positive integer and adds a version chck for that given version. This wont work with Dql though.

EntityRepository should also be changed, this affects:

EntityRepository::find($id, Lock $lockMode = null, $lockVersion = null);
EntityRpository::findOneBy(array $criteria, Lock $lockMode = null, $lockVersion = null);

Entity Repository would translate a $lockVersion != null into a $criteria. Then we onlly need to change:

StandardEntityPersister::load($criteria, $entity, $assoc, $hints = array(), $lockMode = null)
StandardEntityPersister::_getSelectEntitiesSQL($criteria, $assoc=null, $lockMode = null)

The all queries have to be done with DQL to get pessimistic locks imho. A DQL Query with locking would look like:

$q->setHint(Query::HINT_LOCK_MODE, Lock::PESSIMISTIC_READ);

I would also like to see an EntityManager#lock() method. However, after re-reading the spec and testing with Hibernate 3.5.0-CR-2, I'm not sure we really need the *_FORCE_INCREMENT variations. In Hibernate, em.lock() with OPTIMISTIC doesnt do anything (it doesnt need to, versions are compared on update anyway). Even with OPTIMISTIC_FORCE_INCREMENT nothing happens, except the normal version update when the entity changed. Not sure whether this is a bug or intended. PESSIMISTIC_FORCE_INCREMENT works as expected.

So I currently think we should have the following as a start:

final abstract class LockMode
{
    const NONE = 1; // default
    const PESSIMISTIC_READ = 2;
    const PESSIMISTIC_WRITE = 3;
}

The LockMode can then be used either in queries or with EntityManager#lock(). Moreso, these lock modes work independantly of whether the entity is versioned (@Version) or not. PESSIMISTIC_READ would acquire a shared read lock while PESSIMISTIC_WRITE would acquire an exclusive write lock.

What I'm not sure yet about are the following things:

1) Whether to add a query hint or an explicit setLockMode() method + $_lockMode property on the query. I'm tending towards the latter to give a more explicit API for locking (hints are a bit non-obvious and may be overlooked).

2) Whether we need any kind of OPTIMISTIC support in LockMode or *_FORCE_INCREMENT support. I currently think we should start without both.

In my opinion EntityManager#lock() is not necessary for PHP

I think it has a place in long running scripts, where you dont know if you have already a write lock for an entity you have retrieved from the session some time ago, however in PHP you certainly know if a script you are executing needs a read/write consistency lock or not.

This is also where optimistic force increment shines in Java, because you update the version column you are certain you have a read consistent version of your entity. In PHP the time-frame between retrievial and a call to lock() is just unnecessary small and could be solved by directly locking the entity.

This is why i think EntityManager::find needs to be able to set the version also. A long running script in php means:

1. Session A retrieves Entity with Id 1 and Version 1 and displays a form
2. Session B retrieves Entity with Id 1 and Version 1and displays a form
3. Session B retrieves Entity withId 1 and Version 1 and updates it to Version 2
4. Session A retrieves Entity with Id 1 and Version 2 and a lost update happens.

This would be the case without checking the version column. A pessimistic lock in this case is not possible, because it would be lost between point 1 and point 4. However an optimistic lock could check upon retrieval of the entity in point 4, if it is still at version 1.

You dont always know at the point of reading an entity whether you want to lock it or not because these decisions can be made in different layers of the application, even during a single HTTP request. And pessimistic (online) locks are never retrieved in "long-running scripts" / "business transactions", i.e. held during user think time, that would be a concurrency nightmare! And the pessimistic locks in JPA are online locks (select ... for update), not offline locks, so there is no difference between Java and PHP when it comes to the pessimistic online locks (select ... for update).

We are really only talking about pessimistic online locks and optimistic offline locks here. (I dont even know if there is such a thing as an optimistic online lock).

All/Most of the examples on the following blog do not refer to long-running business transactions (transactions that span multiple database transactions with user-think time in between).
http://weblogs.java.net/blog/2009/07/30/jpa-20-concurrency-and-locking

Of course EntityManaher#find et al should be extended with optional lockMode and lockVersion arguments like you said but nevertheless there should still be EntityManager#lock().

Let me clarify, pessimistic online locks only really make sense for the duration of a transaction since they're released at the end of a transaction. You dont want to hold such a transaction open during a long-running business transaction (user-think time / multiple requests). Thats why there is no difference in usage of such locks in Java or PHP.

actually pessimistic (time limited) offline locks are also commonly used.

Moreso, even if you do know you want to lock at the point of reading the entity (which may not be the case like I said above, these decisions can be made in different layers, or even in code of an extension that receives an existing object and now wants to lock it to do its work properly. Without em.lock() that would mean re-reading the whole entity), you may still not want to lock at the point of reading:

1) read entity
2) do some (potentially expensive) computations/calculations/whatever based on the state of the entity but we dont need a lock yet
3) after inspecting the outcome of 2) we decide we want to lock

(yes, locking after reading risks stale data but it locks for a shorter duration, its a compromise)

When you can only lock during reading, this would mean in such a case holding the lock unnecessarily long.

Err .. what you just described is optimistic offline locking. Aka you read the version identifier, go off do your thing, commit .. possibly discover that there was a concurrent write ..

@Lukas: Yea. We're really only talking about pessimistic online locks and optimistic offline locks here. I know there are pessimistic offline locks (D1 even has an implementation of that which I wrote) its not currently in the focus for D2 and can even be provided by an extension.

Ok given layering EntityManager#lock() has use-cases.

However from my experience I want to lock entities at the beginning of long running scripts, because these runs are more important to my business than those small runs from users.

Say i have an entity that at a certain point in time needs to calculate very expensive stuff, before I start the calculations i would want to make sure that these wont get busted by a user updating the entity during that run, so i would apply SELECT * FOR UPDATE before the calculations and not after them. At point 3 when you apply the lock, you only know if you have a consistent lock between 1 and 3 when you have a version field to check against, a pessimistic lock wont help there.

The pessimistic lock in EntityManager#lock() makes sense if you want to make sure that after the aquiring no changes happen, for example:

1. read entity
2. do some simple checks
3. realize after 2, we need to make expensive computations, lock() or refresh(entity, lockMode) here? (probably depends if the entity has a version column or not)
4. do expensive computations
5. flush

Based on all the discussion so far I updated the main issue description with an API specification. Feel free to comment.

Next on my list is working out the transaction semantics for all the cases, i.e. where to enforce that it is invoked inside an active transaction and in which cases the Optimistic/Pessimistic exceptions cause a transaction rollback (or whether there is actually a case in which they do not result in a rollback).

First update for transaction requirements.

Updated refresh() specification.

Add another condition for PESSIMITIC locks in combination with find()*

• Refresh entity with lock when entity with id exists in identity map already

hm actually only EntityManager#lock($entity, $mode) has to be called.

Patch for the said functionality, its missing refresh() changes though.

I made the following observations on lock timeouts:

• MySQL supports them for InnoDb at the Database Configuration level
• Postgres has either NO WAIT or no timeout at all
• Oracle has either NO WAIT, no timeout, or a specified number of miliseconds.
• Sqlite has no locking

Unrelated but:

• MsSql has a very weird row lock syntax, we need a new construct in AbstractPlatform for this.

Question:

Should we support something like lock timeouts via a query hint? If yes:

• Should we only specifiy something like setHint(Query::HINT_LOCK_TIMEOUT_NOWAIT, true);
• or like setHint(Query::HINT_LOCK_TIMEOUT, 0);

Vendors except Oracle and Postgres would simply ignore this option. Postgres would only support TIMEOUT = 0 as NOWAIT

The latest version of lock-support is available here:

http://github.com/beberlei/doctrine2/tree/lock-support

The Lock Support is now tested using Gearman Job Server allowing to have functional scenarios where waiting for lock releases is necessary, see:

http://www.whitewashing.de/blog/articles/129

Refresh() is still missing, I am not sure if this should be included in the 2.0 version (use-case is very slim).

Great work so far. I think we can skip refresh() support for now, so post-2.0 if at all.

@"Should we support something like lock timeouts via a query hint?"

I think setHint(Query::HINT_LOCK_TIMEOUT, 0) would be good. That way we keep the possibility open for later enhancements regarding other timeout values (i.e. if features change on databases) without requiring public API changes. However, it should be clearly documented that this is a hint and not a guarantee and it should be documented which database vendors interpret the timeout in which way I think.
As a start, only having timeout = 0 => NOWAIT would be enough.

You can reschedule the lock timeout / nowait to beta3 if you want that. I think we already have enough for beta2.

Pushing outstanding work back to beta3.

Implemented, Lock Timeouts will be handled in a dedicated ticket.

Sounds good to me, the method name is also very clear about that the retrieved object is partial, the only negative is that its really the only use-case for this method.

I think this has some more use-cases. For example if you only want a single object and you have its ID and all you really need is the ID during that request. Proxy objects are initialized even if you call getId() because we (the proxy) cant know what happens inside that method.

Of course, partial objects are always dangerous and I think this is made pretty clear in the manual but the choice is up to the user in the end.

Another interesting use-case: updating an object without loading it (as an alternative to a DQL bulk UPDATE):

$user = $em->getPartialReference('User', $userId);
$user->setName('newname');
$em->flush();

Of course one needs to be aware that the original entity data in such an update is not "correct", i.e. when using event listeners.

Rescheduling for beta3.

Hello,

this fix needs to be implemented in UnitOfWork on line 612 (function persistNew()) as well - when I try to save an entity, I get an Exception from my error handler:

ErrorException with Argument 2 passed to Doctrine\ORM\Mapping\ClassMetadata::setIdentifierValues() must be an array, integer given, called in /var/www/svn/cWorld_ZF/branches/devel-trunk/library/Doctrine/ORM/UnitOfWork.php on line 612 and defined
Backtrace: #0: Doctrine\ORM\Mapping\ClassMetadata->setIdentifierValues at /var/www/svn/cWorld_ZF/branches/devel-trunk/library/Doctrine/ORM/UnitOfWork.php:612
#1: Doctrine\ORM\UnitOfWork->persistNew at /var/www/svn/cWorld_ZF/branches/devel-trunk/library/Doctrine/ORM/UnitOfWork.php:1247
#2: Doctrine\ORM\UnitOfWork->doPersist at /var/www/svn/cWorld_ZF/branches/devel-trunk/library/Doctrine/ORM/UnitOfWork.php:1210
#3: Doctrine\ORM\UnitOfWork->persist at /var/www/svn/cWorld_ZF/branches/devel-trunk/library/Doctrine/ORM/EntityManager.php:438

The relevant code in UoW is:

            $idValue = $idGen->generate($this->em, $entity);
            if ( ! $idGen instanceof \Doctrine\ORM\Id\AssignedGenerator) {
                $this->entityIdentifiers[$oid] = array($class->identifier[0] => $idValue);
                $class->setIdentifierValues($entity, $idValue);

We're using the SequenceGenerator

@SequenceGenerator(allocationSize=1,sequenceName="address_id_seq")

which doesn't return an array, so the array typehint fails and generates an error.

Possible fix in UoW, line 612:

$class->setIdentifierValues($entity, $this->entityIdentifiers[$oid]);

please open a new issue, or reopen this one. posting into closed issues does more harm than good

Hello Benjamin,

since I've no rights to re-open this entry I had to create a new one: http://www.doctrine-project.org/jira/browse/DDC-714

hm, maybe our user-rirghts arre strange. I look into it later

I think in most cases the entry in the association table should be deleted by the FK constraint. However I think that is currently not the default. You can force it by using onDelete="CASCADE" on the @JoinColumn definitions inside the @JoinTable. That should probably be set by default on join columns on an association table.

I'm not sure its worth supporting manual deletion of association table entries through Doctrine as all databases support proper foreign key constraints and this is the most effective way to delete these entries and also enforces the integrity on the database side.

So I'm rather voting for making onDelete="CASCADE" the default for join columns of association tables unless specified otherwise.

It appears that this doesnt work in JPA(2) either, or well, it depends on the implementation provider. But cascade=REMOVE on @ManyToMany is not allowed per the spec.

This issue here is probably one of the reasons for this. Meaning there might be no easy way for the ORM to deal with entries in association tables transparently on cascade=REMOVE.

We have the following options:

1) Disallow cascade=REMOVE on ManyToManyMapping (throw a MappingException)

2) Allow it but clearly document that entries in association tables are not removed by Doctrine, so that you need to apply onDelete="CASCADE" on the @JoinColumn definitions. Are there any more caveats with this I can not think of currently?

3) Someone has an idea for transparently deleting entries in association tables on cascade=REMOVE. Solutions would need to be performant and not too cumbersome to implement.

Waiting for feedback.

Hm after thinking about it the problem is that the collection has to be either:

1. loaded into memory completely and put through to regular remove a single entity process. This would be required to mark entities for deleted that are also hydrated but attached to another many to many collection of this type.
2. selected for all their ids and there should be a many to many deletion mechanism that works based on id, however then also finds also hydrated entities based on their id.

Hm, I don't know if my report was unclear... What I meant was automatic update of the join table ("user_alert" in this example). When I delete e.g. a User, I don't want to have to care about updates in the "user_alert" table - this table was automatically generated (by the @JoinTable statement), so it should be automatically updated, e.g. when I delete either a User or an Alert...

How about removing a relation between a user and an alert?

Meaning the user and alert both remain in the database but they are not connected anymore (think about users with multiple roles).

@Alexander: Just like in normal OOP. If a user as a collection of alerts and an alert a collection of users (bidirectional many-many) and you want to remove an alert from a user you simply remove the alert from the collection and the user from the collection of the corresponding alert. This is very clean, the only problem with this is the efficiency because in order to remove the elements from the collections, the collections must be loaded. (Strictly speaking, you only need to adjust the "owning side", so only 1 collection needs to be loaded). Of course you can always use some custom SQL that directly manipulates the intermediate table. See also DDC-128. Also there is DDC-45 but I dont think thats doable.

Collections of entities always only represent the association of the entity that has the collection and the contained entities. If you remove an entity from a collection, the association is removed, not the entity itself. "The association is removed" means that either the foreign key is NULLed out (in the case of one-to-many) or that the entry in the association table is removed.

@Roman: All sounds very simple. But in practice this doesn't seem to work.
I have the following:

$uRep = $em->getRepository('System_User');
$user = $uRep->findOneById(1);
unset($user->roles[0]);
$em->persist($user);
$em->flush();

Now, when I clear the manager and reload the user:

• If the user had 1 role -> it still has one role
• If the user had more roles -> it still has the removed role and all others are duplicated.

@Alexander: Please open a separate issue for this. Thanks.

Pastebin entries have expired and this bug is not attached to a specific version so it is difficult to tell the status. Nico, is this still a problem with the current release?

My currently proposed solution to this would be to simply make onDelete="CASCADE" the default for join columns in join tables. Anyone has a strong objection or better idea?

sounds good to me

Other tools actually delete the related records manually, no cascading involved. Since Sqlite doesn't even support foreign keys we should probably do the same instead of relying on "onDelete"="cascade".

Whats your take roman?

We have to support this anyways for the following reason:

Doctrine 2 gives you absolutely no access to the many-to-many join table, i.e. to be working with cascade it should not only be the default, but the only option (since other options don't work). We need to extend the way "delete" works inside each persister to also clean up many-to-many tables.

We need this mechanism anyways for Element Collections to delete all the related entries.

The option onDelete="cascade|noaction" is therefore only a hint for the persisters to decide if they perform this action themselves, or let the database vendor perform it.

I pushed my proposed changes to a feature branch: http://github.com/doctrine/doctrine2/tree/DDC-130

There are definatly refactorings that need to be done, however in that state its currently doing its job very well.

What about DQL DELETE?

DELETE FROM Boo WHERE bar

For each join table:

DELETE FROM join_foo WHERE (SELECT id FROM foo WHERE bar)

Can this be done determinstically?

The DQL stuff is just way to much to add. Foreign Key constraints should fail in these cases imho

I think we have to do this on bulk deletion then, too.

Examples from EclipseLink:

em.createQuery("delete from User2 u where u.id=1").executeUpdate();
=>
DELETE FROM USER2_GROUP2 WHERE EXISTS(SELECT ID FROM USER2 WHERE (ID = ?) AND ID = USER2_GROUP2.User2_ID)
DELETE FROM USER2 WHERE (ID = ?)

em.createQuery("delete from User2 u").executeUpdate();
=>
DELETE FROM USER2_GROUP2
DELETE FROM USER2

Fixed in master using the following semantics:

• If a many-to-many join table is to be found not using onDelete="CASCADE" execute this manually
• In self-referential relationships delete both possible pairs.
• Use Database onDelete="CASCADE" if present on both foreign keys of the join table.

Tested against normal entities, self-referential, CTI and STI, from the owning and the inverse side.

Not sure if the issue is identical but seems at least related. Using NOTIFY change tracking with many-to-many bidirectional associations does not work. Objects added to the associations do not get persisted when calling EntityManager#flush.

Tested on r7404.

Dear Roman,

the line # 456 in UnitOfWork.php seems wrong to me:

            $isChangeTrackingNotify = isset($this->entityChangeSets[$oid]);

Shouldn't this only be set if the entity has

 @ChangeTrackingPolicy("NOTIFY") *

set in his class docBlock? The current behaviour now is to assign $changeset if changes exists, leaving the NOTIFY tracking policy out:

             $changeSet = $isChangeTrackingNotify ? $this->entityChangeSets[$oid] : array();

Because of this change, all our unit tests involving saving of entites break (basically, the whole application), which implement @postUpdate for logging purposes logging an own computed changeset.

Hi,

you're right, I did that naivly because I thought the only case where an entity would already have a changeset on flush is with the NOTIFY policy. I did not think of custom use cases like yours. It is fixed now in master. My apologies. However, this still means your approach would be broken if you would use the NOTIFY policy right? That sounds like maybe there is potential to improve the approach you're currently using. If you're missing anything in the API or implementation that forbids a different approach feel free to raise some new JIRA issues so we can possibly improve the situation.

Thank you for fixing this real quick! Our current approach is to compute the changeset on @preUpdate and after a successful save write the changeset to the database in @postUpdate. This conflicted with the changes made, I will however look into it and see if it's feasible for us to implement another approach.

Once again, thanks for your reply and fix.

Benjaming already made quite some progress on this chapter. More to come for BETA2.

Feel free to close this if you think the DQL docs are already good enough

Cover all the important topics now, refactoring and extension could be another ticket.