Locale module uses an array to store data of translatable projects and available translations during its translation update batch processes. Also different (helper) function are used to process the project related data. Readability and maintainability of the code could be improved by combining functions and data storage in a TranslatableProject class and by using a typed class to store the translation source file data. This was suggested by Berdir in #1804688-14: Download and import interface translations. This gives the opportunity to simplify the way the $source data is handled within the batch operations.
Comment | File | Size | Author |
---|---|---|---|
#44 | locale-project-translation-object-1842380-44.patch | 100.99 KB | Sutharsan |
#44 | interdiff-1842380-43-44.txt | 87.65 KB | Sutharsan |
#43 | locale-project-translation-object-1842380-43.patch | 93.43 KB | Sutharsan |
#36 | interdiff.txt | 40.1 KB | skipyT |
#36 | locale-project-translation-object-1842380-36.patch | 92.33 KB | skipyT |
Comments
Comment #1
Sutharsan CreditAttribution: Sutharsan commentedTagging for extra attention ;)
Comment #2
Gábor HojtsyShould this not be based on Update module representing projects with a class instance first and foremost?
Comment #3
Gábor HojtsyI think this should be postponed on introducing project classes themselves, no? That is #1832946: Runtime translation download fallback works different from installer translation download fallback.
Comment #4
Sutharsan CreditAttribution: Sutharsan commentedAgree
Comment #5
Sutharsan CreditAttribution: Sutharsan commentedSharing my ideas of how to improve the readability and maintainability of the code by aggregating the project translation state and helper functions into one class. The patch is build on top of #1998056: Automatically update interface translations using cron #25 and therefore quires that patch to be applied first.
Comment #7
jair CreditAttribution: jair commentedComment #7.0
jair CreditAttribution: jair commentedUpdated issue summary.
Comment #8
aks22 CreditAttribution: aks22 commentedComment #9
aks22 CreditAttribution: aks22 commentedThe patch file which i was applying is not there.
File path:
/core/modules/locale/lib/Drupal/locale/ProjectTranslationState.php
also checked on git.
Comment #10
aks22 CreditAttribution: aks22 commentedThe patch file is for core/modules/locale/lib/Drupal/locale/ProjectTranslationState.php
which does not exist in core module.
Comment #11
Sutharsan CreditAttribution: Sutharsan commentedThe patch needs to be rerolled first. See https://drupal.org/patch/reroll for instructions.
Comment #12
akozma CreditAttribution: akozma commentedRe-roll.
Comment #13
akozma CreditAttribution: akozma commentedComment #15
akozma CreditAttribution: akozma commentedre-roll
Comment #17
Sutharsan CreditAttribution: Sutharsan commentedUnassigning @akshay.swnt2. If you are still woking of the issue, please tell us here.
Comment #18
skipyT CreditAttribution: skipyT commentedAfter discussing with Sutharsan I'm proposing the next solution:
- we wait until #1842362: Replace locale_project table and improve caching will be committed
- we rewrite the interface for the project storage to return TranslatableProject instances
- we rewrite the interface for project storage to save TranslatableProjects inside the key value store. We can create a toArray() method on the translatable projects class.
- we'll store the translation status for each project inside the TranslatableProject data, this means it will be saved with the other project data in the key value store into the locale.project collection.
I propose also the following class names:
- project storage: LocaleProjectStorage
- translatable project class: LocaleTranslatableProject or just TranslatableProject
Comment #19
Sutharsan CreditAttribution: Sutharsan commented#1842362: Replace locale_project table and improve caching was committed. Lets get this one rolling again ;)
Unassigning akshay.swnt22. This is not personal, but is has been a long time and I like to clear the path for anyone (incl. myself) to pickup the issue.
Comment #20
skipyT CreditAttribution: skipyT commented@Sutharsan: I could help also. you can contact me on IRC to discuss about this. my nickname is: skipyT
Comment #21
Sutharsan CreditAttribution: Sutharsan commentedWorking on rerolling the patch. Major changes in the status form...
Comment #22
Sutharsan CreditAttribution: Sutharsan commentedRerolled the #15 patch, but did not make changes to TranslationStatusForm.php. The code of the form changed significantly and I doubt that the status form changes in #15 will survive. Because I don't want to spent a lot of time re-rolling code that will not be reused, I dropped it in this re-roll.
Comment #23
Sutharsan CreditAttribution: Sutharsan commentedThis is the implementation of the ideas described in #18. Code is partly working, test not yet tested. The interdiff is perhaps too big to be useful.
Comment #25
Sutharsan CreditAttribution: Sutharsan commentedMore working code. Completed interface documentation. Can't get the tests working locally (even on 8.0.x, without patch), so no warranties on tests.
Comment #27
Sutharsan CreditAttribution: Sutharsan commentedMade test working. Some LocaleTranslationProject methods renamed.
Comment #28
Sutharsan CreditAttribution: Sutharsan commentedGo botty
Comment #31
Sutharsan CreditAttribution: Sutharsan commentedComment #32
skipyT CreditAttribution: skipyT commentedHi,
I read your patch and I have some ideas how could we improve the patch. But I need some time to reflect on it.
My concern is with that source object returned as a stdclass. Perhaps we should somehow split this class into 2 separate classes and try to have classes doing only one thing.
Also some methods are not unit testable, like the update method. I will try to push a patch until tomorrow morning to propose a new, cleaner class hierarchy here. I wanted to do it for yesterday, but it took me a while understanding the whole code.
Comment #33
Sutharsan CreditAttribution: Sutharsan commentedLast week I discussed in IRC with SkipyT some possible improvements:
use
$project->getSourcebyLangcode($langcode)->getRemoteSource();
Comment #34
skipyT CreditAttribution: skipyT commentedI tried to rewrite the LocaleTranslatableProject to a new class hierarchy. Now I have:
- LocaleTranslatableProject
- ProjectState
- AbstractProjectSource
- LocalProjectSource
- RemoteProjectSource
This is a work in progress, I uploaded it only to have an early review. I will continue to work on this. I need to create the class interfaces, to clean the code, to have the right comments.
Comment #36
skipyT CreditAttribution: skipyT commentedI continued the work on the patch from #34.
Comment #38
skipyT CreditAttribution: skipyT commentedrerolled the patch.
Comment #39
skipyT CreditAttribution: skipyT commentedComment #41
Sutharsan CreditAttribution: Sutharsan commentedI've discussed at DC Amsterdam, with skipyT how to proceed.
Some code examples as we would like to retrieve the source data.
Comment #42
Sutharsan CreditAttribution: Sutharsan commentedWorking ...
Comment #43
Sutharsan CreditAttribution: Sutharsan commentedFor the record, #38 rerolled.
Comment #44
Sutharsan CreditAttribution: Sutharsan commentedThis patch is partially working. The OO architecture is complete enough for a review. I'm not sure if I have the time to continue working on it the following days, therefore I share what I have now.
Summary of changes:
Still to do:
Comment #52
nterbogt CreditAttribution: nterbogt at Flight Centre Travel Group commentedIs this something that is still on the cards? I'm having all sorts of performance issues with this specific part of locale.
I have 157 custom languages, only 7 of which exist on the Drupal translation servers.