Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removed Id from EnittyTranslation fixture. #158

Merged
merged 2 commits into from Nov 15, 2014

Conversation

@kuczek
Copy link
Contributor

kuczek commented Oct 20, 2014

I removed $id from TranslatableEntityTranslation and now tests will fail.

My question is, what should be a key for EntityTranslation classes. Should user manualy define it, or there should be some predefined (id of Translatable and locale for example)?

@kuczek kuczek changed the title Removed Id from EnittyTranslation fixure. Removed Id from EnittyTranslation fixture. Oct 20, 2014
@kuczek

This comment has been minimized.

Copy link
Contributor Author

kuczek commented Oct 21, 2014

This is connected with #154

@docteurklein

This comment has been minimized.

Copy link
Contributor

docteurklein commented Oct 22, 2014

It's a regression we introduced recently (as explained in a commit discussion).

We'll fix that asap.

@docteurklein

This comment has been minimized.

Copy link
Contributor

docteurklein commented Oct 22, 2014

meanwhile, you can use a tagged (stable version) or provide a specific commit hash in your composer.json.

@docteurklein

This comment has been minimized.

Copy link
Contributor

docteurklein commented Oct 22, 2014

thanks for opening this BTW :)

The only suolution right now is to add a $id property manually in your classes (it will then automatically be mapped as a auto primary key by doctrine).

@kuczek

This comment has been minimized.

Copy link
Contributor Author

kuczek commented Oct 22, 2014

Yes, but how it should be solved? What is desired behavior?

MapId in translationSubscriber is mark as deprecated but is called.
I tried to create primary id as translatable and locale but it's not working without flushing changes before adding translations. (in MYSQL).

@docteurklein

This comment has been minimized.

Copy link
Contributor

docteurklein commented Oct 22, 2014

you can reach the same behavior as before if you add (temporarly)

    protected $id;

in every class that requires it.

In theory this will be detected as a primary key and doctrine will work as before.
You can also decide to define the mapping by yourself (via annotation or anything else:

     /** @ORM\Id @ORM\Column(type="integer") @ORM\GeneratedValue */
    protected $id;
@docteurklein

This comment has been minimized.

Copy link
Contributor

docteurklein commented Oct 22, 2014

all this is temporary, next commits should fix this regression.

@kuczek

This comment has been minimized.

Copy link
Contributor Author

kuczek commented Oct 22, 2014

Yes, but I still do not know how it should work? Maybe I can help with this?

@docteurklein

This comment has been minimized.

Copy link
Contributor

docteurklein commented Oct 22, 2014

the desired behavior is to:

  • provide the $id property (as before, we removed it by mistake). TODO
  • make it so that doctrine maps the id automatically (no annotation required, all is done via loadClassMetadata subscribers). DONE

Would be great if you can propose somehing! thanks :)

@kuczek

This comment has been minimized.

Copy link
Contributor Author

kuczek commented Oct 22, 2014

So why method mapId in TranslationSubscriber is deprecated?

@docteurklein

This comment has been minimized.

Copy link
Contributor

docteurklein commented Oct 22, 2014

mmh, I may repeat what I already told in 75e1187#commitcomment-8254962

Be careful not to duplicate with @bobvandevijver .

@docteurklein

This comment has been minimized.

Copy link
Contributor

docteurklein commented Oct 22, 2014

@kuczek I'm not sure why.

We just introduced it, so it shouldn't be depreacted already :)
I think it's to say that it's a temporary thing, that should be provided by doctrine directly.
Currently, because of a doctrine limitation, we had to copy its code. That's why.

@kuczek

This comment has been minimized.

Copy link
Contributor Author

kuczek commented Oct 27, 2014

Maybe we should make this id optional... Somehow? Now you are unable to override it. Php will allow but doctrine will not.

@docteurklein

This comment has been minimized.

Copy link
Contributor

docteurklein commented Oct 27, 2014

i thought we were adding it only if manual mapping didn't exist.
Otherwise, you can try with @AttributeOverride. Not sure it works with ids tho.

@kuczek

This comment has been minimized.

Copy link
Contributor Author

kuczek commented Oct 28, 2014

So it should look like now?

@docteurklein

This comment has been minimized.

Copy link
Contributor

docteurklein commented Oct 29, 2014

yes

@kuczek

This comment has been minimized.

Copy link
Contributor Author

kuczek commented Oct 29, 2014

So i think it's ready for merge.

@ferdynator

This comment has been minimized.

Copy link

ferdynator commented Nov 15, 2014

When will this be merged? We have to use a pinned version for our current project because of this issue

docteurklein added a commit that referenced this pull request Nov 15, 2014
Removed Id from EnittyTranslation fixture.
@docteurklein docteurklein merged commit 6dbb91c into KnpLabs:master Nov 15, 2014
1 check passed
1 check passed
continuous-integration/travis-ci The Travis CI build passed
Details
@MAXakaWIZARD

This comment has been minimized.

Copy link
Contributor

MAXakaWIZARD commented on 33fbe9b Mar 2, 2015

@kuczek So id field will be automatically mapped by TranslatableSubscriber, no need to map it in translation entity? Because I get notice regarding definition of that field in both trait and entity.

This comment has been minimized.

Copy link
Contributor Author

kuczek replied Mar 3, 2015

@MAXakaWIZARD Yes, you do not need to do nothing with that, it will be mapped automatically.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.