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

Adding Italian language package #21013

Closed
wants to merge 4 commits into from
Closed

Adding Italian language package #21013

wants to merge 4 commits into from

Conversation

WaPoNe
Copy link
Contributor

@WaPoNe WaPoNe commented Feb 6, 2019

Description (*)

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)

@magento-cicd2
Copy link
Contributor

magento-cicd2 commented Feb 6, 2019

CLA assistant check
All committers have signed the CLA.

@magento-engcom-team
Copy link
Contributor

Hi @WaPoNe. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento-engcom-team give me test instance - deploy test instance based on PR changes
  • @magento-engcom-team give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

@WaPoNe
Copy link
Contributor Author

WaPoNe commented Feb 6, 2019

Adding Italian language package

@aleron75 aleron75 self-assigned this Feb 8, 2019
Copy link
Contributor

@aleron75 aleron75 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @WaPoNe thanks for your contribution.

In order to make static test go green, in language pack's composer.json please remove version and set dependency from magento/framework to *.

Finally, add "magento/language-it_it": "*" in the replace node of root composer.json

@hostep
Copy link
Contributor

hostep commented Feb 8, 2019

@WaPoNe: out of curiosity, what does this PR actually achieve? I don't see any translations in your PR?

Maybe I'm missing something obvious here, but I never quite understood what those built-in language packs do in Magento, there's just nothing in there?

@WaPoNe
Copy link
Contributor Author

WaPoNe commented Feb 11, 2019

@aleron75 : Done!

@WaPoNe
Copy link
Contributor Author

WaPoNe commented Feb 22, 2019

Conflicts resolved

@WaPoNe
Copy link
Contributor Author

WaPoNe commented Feb 22, 2019

@hostep : you're right. My PR does not achieve anything special; it just adds Italian language to built-in language packs ;-)

@hostep
Copy link
Contributor

hostep commented Feb 22, 2019

@WaPoNe: ok, and can you explain what this means exactly? Does it help in some way, or how can we see the improvement which is being done by this PR?

@aleron75
Copy link
Contributor

Does it help in some way, or how can we see the improvement which is being done by this PR?

Hello @hostep I see the point of your question but I also think that if other language packs are there in the same format we should wonder what is the value of all of them, not only of this one.

In other words, should we accept this or remove the others?

@hostep
Copy link
Contributor

hostep commented Feb 25, 2019

I agree, never quite understood what those other empty language packs do in core Magento.
If there is no purpose for them, they should probably be removed indeed.

@WaPoNe
Copy link
Contributor Author

WaPoNe commented Feb 25, 2019

My idea reflects the @aleron75 one: you should decide to accept this language pack or to remove the others?
@hostep what is the final decision?

@hostep
Copy link
Contributor

hostep commented Feb 25, 2019

@WaPoNe: well I can't really give a hard decision here, since I'm only a community contributor, not a member of the core Magento team :)

I'm currently trying to understand why these are here in the first place, to understand the history and trying to figure out if they still have a purpose.

So, they got introduced in 974d259
The most relevant part of the commit message seems to be:

* Framework improvements:
  * Introduced language packages with ability to inherit dictionaries

In that commit, I can see some csv files in various modules which still got translated texts, for example, in the csv file for French in the Catalog module: app/code/Magento/Catalog/i18n/fr_FR.csv, there are still translated phrases in there:
"Error during retrieval of option value: %s","Erreur lors de la récupération de la valeur de l'option:% s"

So at that point, these (empty) translations packs probably made the non-English translations from the various .csv files work if I have to guess, since there needs to be some sort of entry point for a translation pack to register itself to Magento.

Then fast forward 17 months, and I find a new commit, where most of these non-English .csv files got removed: 9736c70b800
If I have to guess again: Magento probably didn't want to ship with partially translated phrases and didn't want to maintain these themselves.
Two months before the removal of these .csv files, Ben announced that they were going to work with crowdin for the translations: https://community.magento.com/t5/News-Announcements/Translating-Magento/td-p/16945

Strangely enough the current codebase still contains some of these non-English .csv files, for example:

$ find . -type f -iname 'fr_FR.csv'
./app/code/Magento/Ui/i18n/fr_FR.csv
./app/code/Magento/ProductVideo/i18n/fr_FR.csv
./app/code/Magento/Search/i18n/fr_FR.csv
./app/code/Magento/Msrp/i18n/fr_FR.csv
./app/code/Magento/Swatches/i18n/fr_FR.csv
./app/code/Magento/Developer/i18n/fr_FR.csv

None of these seem to contain translations in French at first sight, just English.
It looks like those got introduced by b9ecb3bc97b, which was merged a day after all the others got removed. I'm guessing there were some merge conflicts here and some new files which were introduced on this branch probably didn't conflict and got introduced without anyone noticing.


Anyways, so far for looking back in the history.

My conclusion is: these empty language packs are probably a left-over from the days where Magento was still maintaining translations themselves. This later got outsourced to the community using crowdin. The idea might have been to regularly pull in translations from crowdin in the Magento core, but until this day, this doesn't seem to have happened and might not happen at all in the future, I have no idea if there are still plans in that direction? (@buskamuza ?)

If there are no further plans from Magento to take care of non-English translations, it would make sense to remove the current language modules from the core, except for the English one, so these directories can probably be removed:

  • app/i18n/Magento/de_DE
  • app/i18n/Magento/es_ES
  • app/i18n/Magento/fr_FR
  • app/i18n/Magento/nl_NL
  • app/i18n/Magento/pt_BR
  • app/i18n/Magento/zh_Hans_CN

Next to those directories, the following files should probably be removed as well:

  • app/code/Magento/Developer/i18n/de_DE.csv
  • app/code/Magento/Developer/i18n/es_ES.csv
  • app/code/Magento/Developer/i18n/fr_FR.csv
  • app/code/Magento/Developer/i18n/nl_NL.csv
  • app/code/Magento/Developer/i18n/pt_BR.csv
  • app/code/Magento/Developer/i18n/zh_Hans_CN.csv
  • app/code/Magento/Msrp/i18n/de_DE.csv
  • app/code/Magento/Msrp/i18n/es_ES.csv
  • app/code/Magento/Msrp/i18n/fr_FR.csv
  • app/code/Magento/Msrp/i18n/nl_NL.csv
  • app/code/Magento/Msrp/i18n/pt_BR.csv
  • app/code/Magento/Msrp/i18n/zh_Hans_CN.csv
  • app/code/Magento/ProductVideo/i18n/de_DE.csv
  • app/code/Magento/ProductVideo/i18n/es_ES.csv
  • app/code/Magento/ProductVideo/i18n/fr_FR.csv
  • app/code/Magento/ProductVideo/i18n/nl_NL.csv
  • app/code/Magento/ProductVideo/i18n/pt_BR.csv
  • app/code/Magento/ProductVideo/i18n/zh_Hans_CN.csv
  • app/code/Magento/Search/i18n/de_DE.csv
  • app/code/Magento/Search/i18n/es_ES.csv
  • app/code/Magento/Search/i18n/fr_FR.csv
  • app/code/Magento/Search/i18n/nl_NL.csv
  • app/code/Magento/Search/i18n/pt_BR.csv
  • app/code/Magento/Search/i18n/zh_Hans_CN.csv
  • app/code/Magento/Swatches/i18n/de_DE.csv
  • app/code/Magento/Swatches/i18n/es_ES.csv
  • app/code/Magento/Swatches/i18n/fr_FR.csv
  • app/code/Magento/Swatches/i18n/nl_NL.csv
  • app/code/Magento/Swatches/i18n/pt_BR.csv
  • app/code/Magento/Swatches/i18n/zh_Hans_CN.csv
  • app/code/Magento/Ui/i18n/de_DE.csv
  • app/code/Magento/Ui/i18n/es_ES.csv
  • app/code/Magento/Ui/i18n/fr_FR.csv
  • app/code/Magento/Ui/i18n/nl_NL.csv
  • app/code/Magento/Ui/i18n/pt_BR.csv
  • app/code/Magento/Ui/i18n/zh_Hans_CN.csv

Next to those, there are some German translation files being used in the integration tests, but I don't think those can be removed:

  • dev/tests/integration/testsuite/Magento/Setup/Module/I18n/Pack/_files/expected/app/design/adminhtml/default/i18n/de_DE.csv
  • dev/tests/integration/testsuite/Magento/Setup/Module/I18n/Pack/_files/expected/app/code/Magento/FirstModule/i18n/de_DE.csv
  • dev/tests/integration/testsuite/Magento/Setup/Module/I18n/Pack/_files/expected/app/code/Magento/SecondModule/i18n/de_DE.csv
  • dev/tests/integration/testsuite/Magento/Setup/Module/I18n/Pack/_files/expected/lib/web/i18n/de_DE.csv

@WaPoNe: maybe hold of a few days until somebody from core Magento can give some feedback here, that might be safest?

@WaPoNe
Copy link
Contributor Author

WaPoNe commented Feb 26, 2019

@hostep : ok.. we'll wait for the safest decision by the Magento core members.

@hostep
Copy link
Contributor

hostep commented Mar 25, 2019

@WaPoNe: after talking a little bit with @vkublytskyi on Slack in the #translations channel, it looks like this PR is not going to get accepted:

I don’t see any real benefits from adding Italian or any other language pack to the core because of one simple reason. None of them except en_US does not contain even a single phrase. And this is actually one of reason why we start the new project with Community as the main goals, for now, are simplify the process of translation and have automated builds and releasing of language packages.

But we also can't remove the current existing language packs:

Removing of them would be backward incompatible change as if some 3rd party module contains translations for language declared in core language package they will be broken. When we will have packages build from Crowdin translations I think we will be able to mark language packages in core as “abandoned” (in composer.json) with reference to new packages

What we can do however is removing that list of non-English csv files inside the various Magento modules. I've just created a PR for this over here: #21927

I hope you don't mind me closing this PR?
Thanks for the effort though! :)

@hostep hostep closed this Mar 25, 2019
@ghost
Copy link

ghost commented Mar 25, 2019

Hi @WaPoNe, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

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

Successfully merging this pull request may close these issues.

6 participants