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.

Files: 
CommentFileSizeAuthor
#44 locale-project-translation-object-1842380-44.patch100.99 KBSutharsan
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,789 pass(es), 161 fail(s), and 20 exception(s).
[ View ]
#44 interdiff-1842380-43-44.txt87.65 KBSutharsan
#43 locale-project-translation-object-1842380-43.patch93.43 KBSutharsan
#36 interdiff.txt40.1 KBskipyT
#36 locale-project-translation-object-1842380-36.patch92.33 KBskipyT
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch locale-project-translation-object-1842380-36.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#34 locale.patch90.45 KBskipyT
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#31 interdiff-1842380-27-31.txt1.21 KBSutharsan
#31 locale-project-translation-object-1842380-31.patch87.63 KBSutharsan
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,171 pass(es).
[ View ]
#27 locale-project-translation-object-1842380-27.patch86.57 KBSutharsan
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,174 pass(es), 1 fail(s), and 2 exception(s).
[ View ]
#25 locale-project-translation-object-1842380-25.patch79.27 KBSutharsan
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,222 pass(es), 74 fail(s), and 4,509 exception(s).
[ View ]
#23 locale-project-translation-object-1842380-23.patch73.13 KBSutharsan
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,034 pass(es), 100 fail(s), and 4,561 exception(s).
[ View ]

Comments

Sutharsan’s picture

Assigned:Unassigned» Sutharsan
Issue tags:+sprint

Tagging for extra attention ;)

Gábor Hojtsy’s picture

Issue tags:+language-ui

Should this not be based on Update module representing projects with a class instance first and foremost?

Gábor Hojtsy’s picture

Status:Active» Postponed
Issue tags:-sprint

I think this should be postponed on introducing project classes themselves, no? That is #1832946: Create a small project API to be used by Update and Locale.

Sutharsan’s picture

Agree

Sutharsan’s picture

Status:Postponed» Needs review
StatusFileSize
new44.15 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch locale-project-translation-object-1842380-5.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Sharing 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.

Status:Needs review» Needs work

The last submitted patch, locale-project-translation-object-1842380-5.patch, failed testing.

jair’s picture

Issue tags:+Needs reroll
jair’s picture

Issue summary:View changes

Updated issue summary.

akshay.swnt22’s picture

Assigned:Sutharsan» akshay.swnt22
Issue summary:View changes
akshay.swnt22’s picture

The patch file which i was applying is not there.
File path:
/core/modules/locale/lib/Drupal/locale/ProjectTranslationState.php
also checked on git.

akshay.swnt22’s picture

The patch file is for core/modules/locale/lib/Drupal/locale/ProjectTranslationState.php
which does not exist in core module.

Sutharsan’s picture

The patch needs to be rerolled first. See https://drupal.org/patch/reroll‎ for instructions.

akozma’s picture

StatusFileSize
new44.49 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,067 pass(es), 110 fail(s), and 84 exception(s).
[ View ]

Re-roll.

akozma’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 12: locale-project-translation-object-1842380-12.patch, failed testing.

akozma’s picture

Status:Needs work» Needs review
StatusFileSize
new44.51 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,606 pass(es), 107 fail(s), and 80 exception(s).
[ View ]

re-roll

Status:Needs review» Needs work

The last submitted patch, 15: locale-project-translation-object-1842380-15.patch, failed testing.

Sutharsan’s picture

Unassigning @akshay.swnt2. If you are still woking of the issue, please tell us here.

skipyT’s picture

After 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

Sutharsan’s picture

Assigned:akshay.swnt22» Unassigned

#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.

skipyT’s picture

@Sutharsan: I could help also. you can contact me on IRC to discuss about this. my nickname is: skipyT

Sutharsan’s picture

Assigned:Unassigned» Sutharsan

Working on rerolling the patch. Major changes in the status form...

Sutharsan’s picture

Rerolled 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.

Sutharsan’s picture

Status:Needs work» Needs review
StatusFileSize
new73.13 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,034 pass(es), 100 fail(s), and 4,561 exception(s).
[ View ]
new69.95 KB

This 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.

Status:Needs review» Needs work

The last submitted patch, 23: locale-project-translation-object-1842380-23.patch, failed testing.

Sutharsan’s picture

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new79.27 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,222 pass(es), 74 fail(s), and 4,509 exception(s).
[ View ]
new26.53 KB

More working code. Completed interface documentation. Can't get the tests working locally (even on 8.0.x, without patch), so no warranties on tests.

Status:Needs review» Needs work

The last submitted patch, 25: locale-project-translation-object-1842380-25.patch, failed testing.

Sutharsan’s picture

Assigned:Sutharsan» Unassigned
StatusFileSize
new86.57 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,174 pass(es), 1 fail(s), and 2 exception(s).
[ View ]
new28.81 KB

Made test working. Some LocaleTranslationProject methods renamed.

Sutharsan’s picture

Status:Needs work» Needs review

Go botty

Status:Needs review» Needs work

The last submitted patch, 27: locale-project-translation-object-1842380-27.patch, failed testing.

Sutharsan’s picture

Status:Needs work» Needs review
StatusFileSize
new87.63 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,171 pass(es).
[ View ]
new1.21 KB
skipyT’s picture

Hi,

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.

Sutharsan’s picture

Last week I discussed in IRC with SkipyT some possible improvements:

  • To replace the stdClass source objects: Create an interface for translation sources and two implementations. One for the local and one for the remote translation source.
  • Instead of using
    $project->setLangcode($langcode);
    $project->getRemoteSource()

    use $project->getSourcebyLangcode($langcode)->getRemoteSource();

skipyT’s picture

StatusFileSize
new90.45 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

I 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.

Status:Needs review» Needs work

The last submitted patch, 34: locale.patch, failed testing.

skipyT’s picture

Status:Needs work» Needs review
StatusFileSize
new92.33 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch locale-project-translation-object-1842380-36.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new40.1 KB

I continued the work on the patch from #34.

Status:Needs review» Needs work

The last submitted patch, 36: locale-project-translation-object-1842380-36.patch, failed testing.

skipyT’s picture

StatusFileSize
new94.02 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
new40.11 KB

rerolled the patch.

skipyT’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 38: locale-project-translation-object-1842380-38.patch, failed testing.

Sutharsan’s picture

I've discussed at DC Amsterdam, with skipyT how to proceed.

  • Keep the stateByLanguage array, it is convenient for data storage/retrieval.
  • Use a typed class(es) for ProjectSource. This will make the code more future proof, will document the properties and it is an obvious candidate for encapsulation.

Some code examples as we would like to retrieve the source data.

<?php
$translationProject
->getName();
$translationProject->getVersion();
$translationProject->getStatusByLanguage('nl')->getRemote()->getTimestamp();
$translationProject->getStatusByLanguage('nl')->setTimestamp($now);
$translationProject->getStatusByLanguage('nl')->isUpToDate();
$translationProject->getStatusByLanguage('nl')->getLatest()->getTimestamp();
$translationProject->getStatusByLanguage('nl')->clear();
$translationProject->getStatusByLanguage('nl')->getRemote()->create($data);
$translationProject->getStatusByLanguage('nl')->hasUpdates();

$translationProject // TranslatableProject
$translationProject->getStatusByLanguage('nl') // Returns TranslatableProject
$translationProject->getStatusByLanguage('nl')->getRemote() // Returns RemoteProjectSource or ProjectSource
?>
Sutharsan’s picture

Assigned:Unassigned» Sutharsan

Working ...

Sutharsan’s picture

Issue summary:View changes
StatusFileSize
new93.43 KB

For the record, #38 rerolled.

Sutharsan’s picture

Assigned:Sutharsan» Unassigned
Status:Needs work» Needs review
StatusFileSize
new87.65 KB
new100.99 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,789 pass(es), 161 fail(s), and 20 exception(s).
[ View ]

This 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:

  • Renamed LocaleTranslatableProject(Interface) to TranslatableProject(Interface)
  • Using TranslatableProject::getStateByLanguage() and TranslatableProject::getState() to access translation state objects.
  • Removed LocaleProjectTranslationState classes.
  • Reworked ProjectSource classes:
    • - ProjectSourceInterface
    • - ProjectSourceBase (formerly AbstractProjectSource)
    • - LocalProjectSource
    • - RemoteProjectSource
  • Using \Serializable to (un)serialize project and source classes.

Still to do:

  • Get all existing tests working
  • Add unit tests

Status:Needs review» Needs work

The last submitted patch, 44: locale-project-translation-object-1842380-44.patch, failed testing.