replaced pre/postBindTranslation events with a postCreateTranslation #572
Conversation
|
ping @ElectricMaxxx / @dbu |
|
Look good, do have to do something in my branch as you extended mine? Or do you merge them together? |
There was a problem hiding this comment.
we can get rid of suppressEvents now i think. a new translation can also be created when flushing, and it should make no difference for the event.
There was a problem hiding this comment.
I dont understand, the method is still being called in cases when the translation already has been bound. One thing that concerns me with the new name is that the event is actually triggered when the translation is bound for the first time. it does not necessarily need to be stored.
Maybe a better name would be pre-/postInitialBindTranslation?
There was a problem hiding this comment.
ups, i should have read the line above, thought you still pass the $suppressEvents into the method. of course, we only want the event on a new translation. maybe s/$suppressEvents/$new ? and if ($new && ...)?
we can go with InitialBindTranslation as name, yes. actually, could it just be one initialBindTranslation event, no pre/post? if you want to change content, you could use prePersist/preUpdate. so this event would just be to track that a translation is being added.
or maybe call it preCreateTranslation and if somebody needs a "real" postCreateTranslation that happens after the flush, he can add that with some sort of queue.
sorry to throw in different ideas here, but if we break things we should figure out what the right scenario is.
makes me wonder if we can somehow postpone this to 1.3 to not block the release. but i guess the problem is that with the refactorings, we have to do something and have a minor BC break of some sort anyways.
There was a problem hiding this comment.
no .. we cannot postpone it as the behavior is already changed so better break BC here only once.
regarding only a single event. not sure. right now at least all events are a pre/post combo.
There was a problem hiding this comment.
as pre and post happen within the same method, they are not a very useful distinction here, thats why i thought about just one of them.
my suggestion would be to only do the preCreateTranslation now and doing no postCreate at all rather than one with a flawed semantics and then need another BC break when fixing that. once needed, a proper postCreateTranslation can be done - if we do it now doing the right thing will become harder.
lsmith77
force-pushed
the
rename_bind_trans_event
branch
2 times, most recently
from
c9354a8 to
d708463
Compare
lsmith77
force-pushed
the
rename_bind_trans_event
branch
from
d708463 to
554598f
Compare
lsmith77
changed the title
|
@dbu: please review |
|
i am +1 on this. @ElectricMaxxx ok with this? the reasoning is that the postCreate should happen after the stuff is flushed, but that would be completely new so we just as well let that to whoever needs that event (if ever). |
|
ok .. merging .. as I want to release. |
replaced pre/postBindTranslation events with a postCreateTranslation
supersedes #571
fixes #569