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?

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

Updating fix version

The Filter API only makes sense for SELECT clauses. Doctrine itself does not use DQL to do updates internally, so you need to use other mechanisms (EventListener) to prevent this operations if they are not allowed for a user.

But I can make custom DQL to update rows and would like to automatically filter this too.

e.g. $em->createQuery("UPDATE SomeEntity se SET se.field = "updated!")->execute();

The lifecycle events preUpdate etc. are not called when doing custom DQL queries.

Maybe it is bad practice and discouraged to do updates, inserts and deletes as custom DQL queries, but I would like to ensure that the other people in my organization can't accidentally bypass the Access Control, even if they make use of such bad practice.

And if the filter API only makes sense for Select statements, why are filters applied to update/delete/etc. statements too?

Well, they are applied to DQL UPDATE/DELETE. But not not UPDATE/DELETE that works through the internals of Doctrine. So yes, you can use it to filter DQL DELETE/UPDATE, but doctrine does not do that internally.

So you have to have two strategies, a DQL/SQL Filter - and Lifecycle events.

Which is fine by me. I already implemented the checks using lifecycle events before opening this issue. The access control is automatically handled when using the entitymanager and not custom DQL.

Now I would also like to filter the custom DQL, but currently I can't, because as originally stated, the filter needs to know which type of query it is being applied to.

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!

Windows

Thats why you shouldn't generate proxies at runtime. The problem happens on windows, because the atomic rename operation doesn't work as perfectly there as on linux.

We cannot fix this in Doctrine.

Benjamin Eberlei What do you mean "you shouldn't generate proxies at runtime"? I'm not in production, this is in dev. And I'm using the default configuration.

What I don't understand is why will Doctrine regenerate proxies on every request? The warning is reproductible, and even when no PHP entity has been touched.

Benjamin Eberlei To simplify my previous message (I don't want to bury you under questions) I'll sum it up like that:

What can I do?

Thanks!

Benjamin Eberlei ping: what can be done?

Can we suppress the error with @rename($tmpFileName, $fileName); ?

https://github.com/doctrine/common/blob/master/lib/Doctrine/Common/Proxy/ProxyGenerator.php#L287

I can make a PR if you think that's a valid solution.

Matthieu Napoli no, if you have warnings, please disable them via ini setting. With error suppression there, we may have further problems identifying more serious issues.

About proxy generation: that happens EVERY time in dev environments. Generate them once and disable it afterwards.

Marco Pivetta OK I can disable the auto generation then (I'll have to remember to regenerate them when I edit the model).

Thanks for the feedback

Is that possible to make those proxies generate only if the entity file has been modified since the last generation? (only asking if can and should be done, I can look for implementing it myself if that's the case)

Matthieu Napoli that would be very obnoxious when changing entities often. I wouldn't do that (generating only if not already available)

Marco Pivetta Yes but for now they are regenerated at every request when in dev mode (at least with the default configuration http://docs.doctrine-project.org/projects/doctrine-orm/en/latest/reference/configuration.html#obtaining-an-entitymanager)

Which one is worse: generating every proxy class at every request, or generate only those which changed (in dev environment of course, not prod)?

If neither of these options are good (i.e. auto generation should be disabled), I don't understand why the docs say to enable auto generation when in dev environment.

Matthieu Napoli that's because in dev environments you shouldn't care about that one exception (usually happens when you got concurrent requests).

It is worse to generate only on changes: that's a lot of additional checks, variables to keep in memory and additional logic that is not needed.

Let's keep it as it is (generating at each request) for dev environments: works fine

Another (eventual) solution for dev environments would be not to write the proxy file, but to eval it.

eval it would be a good solution IMO, no more "woops the directory is not writable" and it's more neutral for the user filesystem (but not as easy to debug). But OK, I see what you mean, it works let's keep it that way.

Actually the problem on my setup is that PHP errors are turned into exceptions, so on an (poorly designed) AJAX treeview (lots of nodes to load => lots of requests), I end up with some nodes not loaded because of the exception. And it feels weird to either silently log all PHP warnings or silently ignore the specific warning for the rename.

Matthieu Napoli I'd go with `eval` then. Needs refactoring of the abstract proxy factory and of the proxy generator (proxy generator should no longer write files).

Re-opening: the proxy factory could directly `eval()` the produced proxy code. The ProxyGenerator should no longer write the generated files to disk automatically.

I've opened a PR: https://github.com/doctrine/common/pull/269

The problem is that changing ClassMetadata after generating it from the cache is not really supported and depends on the Internal State of other classes. Have you tried creating a completly new EntityManager and then directly setting this? It could be that the SQL for the entity was already generated inside Doctrine, with the ID Generator information at IDENTITY_AUTO.

> The problem is that changing ClassMetadata after generating it from the cache is not really supported

Yeah, it is a problem indeed, why set ticket status to resolved ?
Do you think it's normal to have a public method that trigger a fatal error ?

Please fix it or put setIdGeneratorType as private, or AT LEAST add a context exception ...

Almost every interaction with metadata outside the `loadClassMetadata` event will cause unexpected problems. I don't think throwing an exception there helps in any way.

@marco pivetta

The generation of the actual exception comes from DBALException on the query excetion and point a bad generated query (Invalid parameter number),
when the problem comes from setting ClassMetada, and concerns a problem of cache generated after loadClassMetadata.

Adding an exception is just the fast way pointing where the problem comes from and that "setting metadata after loadMetadata is not supported anymore". (It will spare developper's time that used to set metadata, but also help future contribution)

> Please fix it or put setIdGeneratorType as private, or AT LEAST add a context exception ...
Note: BTW, my favorite solution would be to fix it (re-generate cache, or edit cache, or disable cache or whatever)

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);
        }
    }
}

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

BC break without advantages

A related Github Pull-Request [GH-673] was reopened:
https://github.com/doctrine/doctrine2/pull/673

Can this be somehow worked around? If not, it is really serious problem...

I couldn't find any workaround.
Trying to force the 'type' condition in the join clause resulted useless as Doctrine would add the 'where' condition regardless.

Easier way to workaround right now, is to declare a OneToMany from class A to class B on a protected field (no need of getter or setter). That way you can do classic join via relationship transversing and then the condition will be placed in the ON part of the query.

I could access with below codes. You should use RAW SQL it is easy solution I guess. good luck.

$qb = $em->createQueryBuilder();

$qb->select('a, b')
->from('YourEntity1', 'a')
->leftJoin('YourEntity2', 'b', \Doctrine\ORM\Query\Expr\Join::WITH, 'a.id = b.relationId');

$raw_sql = $qb->where( 
	$qb->expr()->in('a.relationId', $ids)
)
->orderBy('a.updatedAt', 'DESC')
->setMaxResults(10)
->getQuery()->getSQL();

$conn = $em->getConnection();
$stmt = $conn->query($raw_sql);

/* $stmt->fetchAll(); // access as Array */

Assigned to Alexander

Duplicate of DDC-1940

Benjamin: this bug doesn't seem to be a dupe of DDC-1940. Actually that issue doesn't seem to be a bug at all.

As a reminder, the problem in this issue is that when performing arbitrary left joins on entities that are part of a class hierarchy, the discriminator condition is placed in the where clause instead of the join clause. This means that rows that could not be joined will have null values in the discriminator column and thus will not be returned because of the where condition (which will contain something like " where x.discriminator in (1,2,3) ").

Has this issue been resolved elsewhere? From reading over DDC-1940 it doesn't seem to be a duplicate at all. I'm experiencing the same problem as Jordi and can't seem to find a solution. Is there any particular reason the `IN ()` predicate is not a part of the join, but instead placed in the main `WHERE` clause?

I've been looking into the root cause of this bug (or feature..) to try and understand why it's happening, and after trying various possible fixes (a little hard without full understanding of the Doctrine/Query internals) I've ended up with a fix that seems to work well for my use case, at least. I've not run any of the unit tests (I plan to, and may adjust my fix based on that) but the top revision is my change... https://gist.github.com/tarnfeld/a6bb50ec707c7af1c5dc/revisions

Would love some feedback.

Pull request here: https://github.com/doctrine/doctrine2/pull/656

Tom's fix moves the condition of the discriminator column to the LEFT JOIN, which is exactly what was needed.

Alexander: could you please give it a look?

Tom's proposal seems to fix the issue.

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.

This is not a bug, but expected behavior.

You can select against the alias if its on the owning side of the association:

SELECT img
FROM Image img 
WHERE img.tag=:tag

In this case it is not a left join though, if you want a left join you HAVE to join.

There is no owning isde of the association, you can clearly see there is an association table/entity.

I can't understand how this behavior is expected. If no properties of Tag are selected for, there is no need to join against Tag since the id is already available via the association table.

I misread the mappings, sorry, i though its a @OneToOne but its actually an assocition entity with @OneToOnes.

Can you show me the Image::$tags mapping also?

That is correct, thanks for taking another look at this.
Sorry I had forgotten to include that information.

Image
    /**
        @Id
    */
    $id

    /**
        @ManyToMany(targetEntity="Tag")
        @JoinTable(name="Tag_Image", 
                            joinColumns={@JoinColumn(name="image")},
                            inverseJoinColumns={@JoinColumn(name="tag")})
    */
    $tags

The targetEntity is wrong. I suppose it should be Image_Tag or not? If it should be Tag, then you don't need that Image_Tag entity at all.

In that case i have to check if you can use the shortcut notation, however it will again not work with the left join - only inner. This is an assumption the ORM makes and there is not yet code included for the optimization. This is not a bug, but an improvement ticket. The functionality works.

No argument on the ticket type...

Ahh, I store some metadata in Tag_Image, which is why I manage it explicitly.

In any case thanks for looking at this.

If you change the targetEntity to Tag_Image then it might already be enough to get this working without another join.

With this change, the original query is invalid:

    LEFT JOIN i.tags t
    WHERE t.id=:tag

Because i.tags of type Tag_Image has no field id

[Semantical Error] line 0, col 138 near 'id=:tag ': Error: Class domain\Tag_Image has no field or association named id

I attempt the logical modification:

    LEFT JOIN i.tags t
    WHERE t.tag=:tag

and get

SELECT i0_.id AS id0
FROM Image i0_ 
LEFT JOIN Tag_Image t1_ ON i0_.id = t1_.image 
LEFT JOIN Tag_Image t1_ ON t1_.id = t1_.tag 
WHERE t1_.tag = 37

SQLSTATE[42000]: Syntax error or access violation: 1066 Not unique table/alias: 't1_

Already merged into core.

But it is not enabled yet Plus we need tests to verify this works in high concurrency enviroments and does not pass the same id twice.

Furthermore the DAtabase Platform Methods are completly missing. No implementations yet.

Schema-Tool support is also missing.

Hi. That is right. Foreign keys (join columns) must point to primary keys, not arbitrary other columns, whether they're unique or not, Doctrine does not know.

In other words, joinColumn must always refer to an identifier/pk. I'm not sure but I think anything else would be a pretty strange relational model, too, but there may be usecases we have not yet encountered.

I'm afraid this will not be possible and would be very hard to implement. Of course if somebody has a patch we happily accept it (after reviewing).

Leaving this open in the case somebody wants to work on it.

A minimal test case. Removing the first flush produces the same error, so this seems to be a bug on inserts as well.

Its not really a bug but rather a new feature This was not intended to work so far.

Ah, ok. Maybe it didn't work before. I don't know where I got the idea that it did.

Thanks.

Oops, closed it before I noticed you said you wanted to leave it open.

Thanks for the testcase though, it is useful. In your concrete example, is it not an option to make the sysname the @Id ?

Yes. That is definitely the way it should be done in this case. I can't really think of a case to have a reference to a unique key while still having an Id on it, except when you're working with an existing, poorly designed database (which is our case).

The reason I assumed this was possible is that the references actually work for lazy loading, but as soon as you start changing the references it throws this error.

Lowering priority.

although we would also need this i would suggest adding an error message if the associated column is not found in $newValId. (class BasicEntityPersister.php _prepareUpdateData)

otherwise the field is populated with null leaving the developer debugging an hour :-/

thx