The following patch adds translation support from salsa to salsa_entity objects in drupal.

Comments

Status: Needs review » Needs work

The last submitted patch, salsa_entity_translation_1.diff, failed testing.

LukyLuke_ch’s picture

Status: Needs work » Needs review
StatusFileSize
new27.68 KB

Fixed the failed tests and added some simple ones for the translation (based on the event) in two different languages.

Status: Needs review » Needs work

The last submitted patch, salsa_entity_translation_2.diff, failed testing.

LukyLuke_ch’s picture

Status: Needs work » Needs review
StatusFileSize
new30.76 KB

More precise mock checks to prevent the simpletest failures here.

berdir’s picture

Status: Needs review » Needs work

Thanks, here's a review :)

  1. +++ b/includes/salsa_entity.entity.inc
    @@ -88,4 +96,57 @@ class SalsaEntity extends Entity {
    +   * Implements Entity::getTranslation().
    

    This can't implement Entity::getTranslation() because that doesn't exist :)

    Needs actual documentation.

  2. +++ b/includes/salsa_entity.entity.inc
    @@ -88,4 +96,57 @@ class SalsaEntity extends Entity {
    +    // Check for cached data.
    +    $cache_key = 'salsa:translation:' . $iso693_3 . ':' . $this->getSalsaType() . ':' . $this->identifier();
    +    $cache = cache_get($cache_key, 'cache');
    

    Ok, so we cache per object now, will need to do some profiling to see how many calls we add due to this, should be ok with caching, I think. However, as we have a lot of cache entries (possibly), we should probably add our own cache table.

    See token_schema() and token_update_8000() I think for how to add that, then use cache_salsa instead of cache.

    Second, the cache API doesn't persist locally, every time you call it, it will load the cache from the database again.

    Extend it with a local cache in $this->translations or so, first check that, then fall back to cache_get(), then fall back to a salsa query.

  3. +++ b/includes/salsa_entity.iso693-3.inc
    @@ -0,0 +1,43 @@
    +/**
    + * @file
    + * Static helper class to get the ISO-693-3 language code from a given ISO-693-2.
    + */
    

    move this to the class docblock, make this just Contains ClassName.

  4. +++ b/includes/salsa_entity.iso693-3.inc
    @@ -0,0 +1,43 @@
    +/**
    + * Class Iso693
    + */
    +class Iso693 {
    

    We need to namespace class names as long as we don't have namespaces, so make it SalsEntityIso693.

  5. +++ b/includes/salsa_entity.iso693-3.inc
    @@ -0,0 +1,43 @@
    +   * returns a ISO-693-3 code for the requested language.
    

    Returns (uppercase)

  6. +++ b/includes/salsa_entity.iso693-3.inc
    @@ -0,0 +1,43 @@
    +   * @param string $langcode
    +   *   Optional: ISO-693-2 Code language - 2-char
    

    format for optional is: (optional) Explain what it is, defaults to X.

  7. +++ b/includes/salsa_entity.iso693-3.inc
    @@ -0,0 +1,43 @@
    +  public static function get($langcode = NULL) {
    

    get() is too generic, it doesn't tell me what it does/return.

    And, if we add this, then we should allow to convert in both directions. We've had use cases for that before.

    toIso693() and fromIso693() maybe?

    If we have it in the method, we could name the class SalsaEntityLanguageMapper or so?

  8. +++ b/includes/salsa_entity.iso693-3.inc
    @@ -0,0 +1,43 @@
    +  private static $code = array(
    +    'de' => 'deu',
    +    'en' => 'eng',
    +    'fr' => 'fre',
    +    'it' => 'ita',
    +    'es' => 'spa',
    +    'ru' => 'rus',
    +  );
    

    property declarations should be above methods. I also think this is not complete, the salsa UI has like 20 languages in the translation popup?

  9. +++ b/salsa_entity.info
    @@ -8,6 +8,7 @@ dependencies[] = entity
     files[] = salsa_entity.inc
    +files[] = includes/salsa_entity.iso693-3.inc
     files[] = includes/salsa_entity.ui.inc
    

    Why does the filename have -3 and the class name not? (might change anyway according to suggestions above.

  10. +++ b/tests/salsa_entity.test
    @@ -60,3 +60,58 @@ class SalsaEntityAPITestCase extends SalsaEntityBaseTestCase {
    +  public static function getInfo() {
    +    return array(
    +      'name' => t('Test salsa translations'),
    +      'description' => t('Check if the translations made in Salsa are loaded and processed correctly.'),
    +      'group' => t('Salsa API'),
    +    );
    +  }
    

    getInfo() should be above setUp()

  11. +++ b/tests/salsa_entity.test
    @@ -60,3 +60,58 @@ class SalsaEntityAPITestCase extends SalsaEntityBaseTestCase {
    +  function testGetTranslationGerman() {
    ...
    +  function testGetTranslationFrench() {
    

    By making two methods, you a) make the test twice as slow and b) don't verify that per-language caches work properly, because this is a completely different installation.

    You also don't need the permission bit, as you just access the event directly.

berdir’s picture

Issue summary: View changes
StatusFileSize
new31.2 KB

Re-rolled, review still needs to be updated.

berdir’s picture

Status: Needs work » Needs review
StatusFileSize
new40.84 KB
new13.74 KB

Re-roll.

1. Ok, it actually does although it's a bit different. So left that.

Everything else should mostly be addressed, including a cache table.

I also added a cache clear button to specifically clear translations and took the opportunity to convert it from a separate local task to a fieldset on the main salsa_api settings page. And cleaned up the submit callbacks, we no longer need to clear the entity_info cache or rebuild the menu, there are no settings left that can affect this.

berdir’s picture

Status: Needs review » Fixed

Committed and pushed!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.