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

Do not translate the main record ID #70

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

aschempp
Copy link
Member

@aschempp aschempp commented Feb 2, 2021

As suggested in #69, this fixes the issue of a translated model ID. Translating the model ID is likely to cause downstream issues e.g. if someone uses $GLOBALS['TL_MODEL'] without knowning of DC_Multilingual.

The existing MultilingualTrait::getLanguageId() should continue to work, as the langPid is still present (but possibly the same as id).

The only possible issue would arise if someone expects $model->id to be the translated record ID, but I cannot imagine why that would ever make sense.

/cc @Defcon0 @dmolineus

@dmolineus
Copy link
Contributor

I depend on the translation id of a translated model. In my setup I have translated content elements for each translation. Therefor I need to get the translation id. Changing the behaviour would crash some of my projects relying on the current implementation.

As long there is a way to get the (internal) translation id and the change belongs to a new major, I'd fine to change it again.

@qzminski
Copy link
Member

Any decisions on this?

@aschempp
Copy link
Member Author

Not really… I hate to release v5 just because of this minority again, but changing it in v4 isn't a good option either 😢

@qzminski
Copy link
Member

It can't be hanging like this forever, if v5 is a must then it's a must.

@aschempp
Copy link
Member Author

Well I fixed it for me by doing an if-else with v4 😇

@fritzmg
Copy link
Contributor

fritzmg commented Mar 31, 2021

Well I fixed it for me by doing an if-else with v4 😇

How/where did you fix it? The current state causes a lot of confusion for our customers :( (in regards to the news_categories extension for example).

@qzminski
Copy link
Member

@aschempp can you shed some light on your solution? Or just merge this PR in v5? 😅 who cares what version is it anyway.

@rabauss
Copy link
Contributor

rabauss commented Mar 31, 2021

Maybe @aschempp uses a private fork - like we do in one project, because we also needed the untranslated ID

@aschempp
Copy link
Member Author

aschempp commented Apr 7, 2021

isotope/core@9d3e457 and isotope/core@feff2d9 (according to the commit log)

@fritzmg
Copy link
Contributor

fritzmg commented Apr 7, 2021

@qzminski would that help for fixing codefog/contao-news_categories#186 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants