add several missing subscribers

[greg0ire]

Completely untested, I lack time, sorry.

[docteurklein]

how responsive :) No problem, people will continue to use annotations for them, and basta.
there is one in sortable too. maybe others.

[greg0ire]

Responsive but also too hasty sometimes. Here is the mapping for sortable.

[docteurklein]

Tests exist already for those cases, but they don't register those new subscribers.

A good test to see if they work would be to remove all annotations, and simply register correponding subscribers.
It should still work after that.

[greg0ire]

The real problem is I forgot to remove the annotations, like I did previously. Fixing that.

[greg0ire]

Also, I did not use the good trait, it should be node, not tree.

[greg0ire]

And I forgot TranslationProperties. I'll do all this later, sorry.

[docteurklein]

no problem :) we have time.

[greg0ire]

I'll stick with TreeSubscriber, though, because it's a subscriber for the Tree behavior more than for the Node trait.

[greg0ire]

Houston ? We have a problem… it is not possible to map the id completely, especially to reproduce the effect of @ORM\GeneratedValue(strategy="AUTO"), because the actual strategy, (sequence generation, auto_increment, or other platform-specific strategies) is decided before the subscribers are loaded and the method that makes this decision is private.

Here are 2 solutions I could think of, tell me which you prefer :

1. Keep the id mapping and tell yml and xml that they need to map this field themselves. This is more BC-compatible
2. Remove the id mapping, the getter / setters and stop telling everyone in the README to remove from their mapping when they use ./console doctrine:generate:entity

I went ahead with 2. If you choose 1, I shall remove cfc9678 and amend e26de5e to add some doc.

[greg0ire]

ping @docteurklein

[greg0ire]

ping @docteurklein

[docteurklein]

Ok, so what are the consequences of option 2 ? They'll have to create ids properties and getters by hand/doctrine:generate:entity ?
In that case, they'll still have to map them in yaml/xml/annot (which is like case 1).
Why not simplify the end user work, and keep them ?
(in which case (option 1), they just have to add mapping, not code and mapping).
What do you think ?

(PS: just noticed the PR is misnamed; it's not anymore just about Tree).

thanks :)

[docteurklein]

BTW @greg0ire, are you talking about https://github.com/doctrine/doctrine2/blob/0bff6aadbc9f3fd8167a320d9f4f6cf269382da0/lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php#L508 ?

In that case, I guess it's still possible to copy what this method internally does (at least we can try), which would give us option 3:
if id is not manually defined, try to define it à la mano :)

[greg0ire]

I fixed the PR name.

Ok, so what are the consequences of option 2 ? They'll have to create ids properties and getters by hand/doctrine:generate:entity ?
In that case, they'll still have to map them in yaml/xml/annot (which is like case 1).
Why not simplify the end user work, and keep them ?

Yes, they'll have to create ids properties and getters by hand / doctrine:generate:entity, but this is two very different cases : in the former case, we are simplifying the end user some work. In the latter, though, we are not : the id is automatically mapped when using doctrine:generate:entity (it does not even ask you if you want an id field or not). This means they have to remember to remove the id mapping when using the trait, so it's actually more work for the end user. Plus, people who do the mapping / create their entity by hand often don't start from scratch (at least it's my case) : I think they often copy an existing entity they already have, so they probably will have more work too. They can't use the trait. Finally, removing the N.B. warning from the README feels good, doesn't it ? It means people need to be less careful when they use the library.

Regarding 3, it is the piece of code I was talking about, and I have been thinking about this solution, but dismissed it : it would copy / pasting code that can evolve, which means that if it suddenly does something new, we should make a new version of the library AND maintain the older one. Duplicate code is never good, especially when it is that big.

Once again, it is your library so it is your decision, I won't argue further. Pick a solution and I will try to do it.

[docteurklein]

thanks for feedback!

I agree that removing the warning in the readme is a good thing.

I would still try to see what option 3 would lead to:
I agree that copying id metadata factory is not the best, but it's cheaper for one developer to copy/paste than for an end-user to configure each and every entity he makes :)

Moreover, we wouldn't have to copy all the code, only the part that we need (i.e: "sequence/column generated strategy").

[docteurklein]

I think it's a bug from doctrine to not allow id metadata config in the event. On future versions it may be possible, and thus removing the need for us to copy/paste impl.

[greg0ire]

Ok, so if we're going with 3, I think we should make sure that bug is reported, maybe suggest that they change the access level from private to something higher, and I also think that maybe an E_USER_DEPRECATED should be thrown when people use the portion of code that maps the id for them.

[greg0ire]

Ok, I'm done with the soul-crushing process of copy / pasting doctrine code right in the library. Don't look at it, if you don't want to get nightmares. I did not add the E_USER_DEPRECATED error though, because it makes the tests break. Can you do it ? Also, can you merge this ?

EDIT: I don't understand why travis breaks. It works for me.
EDIT 2: Aha! I get the same error after updating my deps.

[greg0ire]

@docteurklein : Now, it should be better. But see how using slightly outdated code breaks everything ? I think we should at least trigger the E_USER_DEPRECATED error.

[greg0ire]

@docteurklein : I just updated the upgrade file, but I don't know what you want me to change in the README.md file. What about the travis build matrix ? How does it work ?
EDIT: I think I found what you are talking about. Very interesting.

[greg0ire]

I rebased on the latest master, and strangely, it does not break for 2.2 or 2.3 . Good to merge then ?

[greg0ire]

ping @docteurklein

[docteurklein]

@greg0ire should it fail ? maybe the examples (fixtures) we use in tests have a mapped id ?

[greg0ire]

@docteurklein : no they do not have a mapped id, I thought it should fail because I remember having slightly outdated doctrine code (see this comment), copying it into doctrine behaviors, and having the tests pass locally (as expected), then pushing the changes here, and seeing travis fail because he got more recent doctrine code that was no longer compatible with the code I copy / pasted. I was not expecting the new doctrine code I copy/pasted over the old one to solve this particular problem to be retro-compatible with 2.2 or 2.3 . But it looks like it is, so, cool!

[greg0ire]

@docteurklein : If you want to be reassured about that, run

bin/phpunit --coverage-html /tmp/whatever && cd /tmp/whatever && php -S localhost:8000

Open your browser at the right address, and the gorgeous UI should show you that the mapId() method is called (it is partially covered).

[docteurklein]

good! so everything's ok, let's merge this!

[greg0ire]

Yay! Go! Go! Go!

[adrienrusso]

https://github.com/KnpLabs/DoctrineBehaviors/blob/master/src/ORM/Translatable/TranslatableSubscriber.php#L163-Lundefined

The property $this->em is undefined at line 163 and remove id mapping create BC break.

[greg0ire]

@adrienrusso : see #147

[greg0ire]

You know ? So you're here to complain and that's all ? If you want stability, don't use the dev-master version! And if you're going to complain, make correct sentences, don't write nonsense like

Property missing and remove id mapping break the backward compatibility...

This is free software and we are doing a lot to improve stability (see #148 and #141). If you don't want this functionality to break again, you're welcome to add some tests.

Anyway, mapping the id is hard and I don't think it should be the concern of this library (unless and Identfiable trait is written in it). This piece of code was copy / pasted from doctrine, and I think you should not rely on it : you should map ids yourself. IMO, it should be removed because it adds too much complexity to a class that should be focused on mapping translation-related fields.

[greg0ire]

@adrienrusso Oh, after a closer look I see you're pointing to a line just below another line where there is still $this->em. Well, if you want to be constructive, you can write a P.R. to fix this. It should be easy.

[adrienrusso]

Remove the mapping make sense but may be branch alias must be incremented because ~1.0 => de-master

[greg0ire]

@adrienrusso : it is supposed to be BC-compatible so no, it should not be incremented. If you don't want to get the dev version, make sure you either use stable as minimum-stability or that you have prefer-stable set to true.

[adrienrusso]

@greg0ire #149

[greg0ire]

Thank you!