Due to some dev-setup misconfiguration, I just spent too much time trying to figure out why my file translations wouldn't work.

The current implementation of TMGMTFileTranslatorPluginController::requestTranslation just silently fails to submit the job:


    if (file_prepare_directory($dirname, FILE_CREATE_DIRECTORY)) {
      $file = file_save_data($export->export($job), $path);
      file_usage_add($file, 'tmgmt_file', 'tmgmt_job', $job->tjid);
      $job->submitted('Exported file can be downloaded <a href="!link">here</a>.', array('!link' => file_create_url($path)));
    }

I couldn't find any error handling indicators in the TMGMTTranslatorPluginControllerInterface, so wanted to check back about the right approach to fix this?

CommentFileSizeAuthor
#6 2198527_tmgmt_file_error-6.patch1.03 KBparanojik
#2 2198527_tmgmt_file_error.patch2.04 KBdasjo

Comments

blueminds’s picture

blueminds commented

Yes, good point.

See how Gengo plugin handles it, that is currently the proper way how to deal with errors during the process:
http://drupalcode.org/project/tmgmt_mygengo.git/blob/HEAD:/tmgmt_mygengo...

dasjo’s picture

dasjo
he/him
Primary language German
Location Zurich
commented
Status: Active » Needs review
StatusFileSize
new 2198527_tmgmt_file_error.patch2.04 KB

Thanks for the hint blueminds!

Attached is a patch that does so. Maybe, the interface should also be defined a bit more precisely.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

I would simplify this a bit and just add the exception message as the rejected job message and return, instead of the exception loop. This might be useful as a generic handling in the job where we call requestTranslation(), but the indirection here seems unecessary.

The message is then also automatically displayed and logged for the job, and I think the prepare directory function logs on it's own what the problem is, so that shouldn't need a watchdog message.

dasjo’s picture

dasjo
he/him
Primary language German
Location Zurich
commented

i'd actually prefer to move the exception handling up to where requestTranslation() is being called

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work

Yes, either that or just set the job rejection message directly. I'm fine with either, setting it directly would be the easier patch, though.

paranojik’s picture

paranojik commented
Status: Needs work » Needs review
StatusFileSize
new 2198527_tmgmt_file_error-6.patch1.03 KB

...this sets the job rejection message directly.

Status: Needs review » Needs work

The last submitted patch, 6: 2198527_tmgmt_file_error-6.patch, failed testing.

kristen pol’s picture

kristen pol
she/her
Primary language English
Location Santa Cruz, CA, USA
commented

The patch from #6 worked as expected. Thanks! I had been scratching my head why my files were showing up.

I am a bit confused on how the messages are dealt with in TMGMT though. Normally, we would use t() when passing a message to drupal_set_message. We are not doing that in this patch for the reject() function and I looked at when the message is actually rendered and it does not use t() there, e.g.

    if ($text = $message->getMessage()) {
      drupal_set_message(filter_xss($text), $message->type);
    }

so it does not appear that any of these messages handled via the TMGMT messaging system allow for translations which seems a bit odd.

Is this correct? I see that the messages are added to the tmgmt_message table and, thus, it would not make sense to translate them when passing the string into the reject() function. But, it seems like it should be passed through during the display process, e.g.

    if ($text = $message->getMessage()) {
      drupal_set_message(filter_xss(t($text)), $message->type);
    }

If that makes sense to others, I can make a separate issue for that.

I think for this patch, it might be better to make it simpler so it doesn't have the embedded variables. Perhaps:

    else {
       $job->rejected('Job has been rejected. Failed to prepare directory @directory.', array('@directory' => $dirname), 'error');
    }

This way the TMGMT messaging system will be consistent (not passing in any t() text). And, if we update the system to use the t() upon rendering then it will get handled at that point.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented

Yeah, the only valid location to apply t() would be when displaying.
addMessage() leads to storage of the message and thus can not yet be translated.

Passing dynamic data through t() is against its design. We store placeholders separately, so translation should be possible, but we need to discuss that separetely. Until now it was not that important. ;-)

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

You forgot to check in one place :) getMessage() is exactly where t() is called:

  public function getMessage() {
    $text = $this->message;
    if (is_array($this->variables) && !empty($this->variables)) {
      $text = t($text, $this->variables);
    }
    return $text;
  }

But you are right, the patch shouldn't mix t() and not using t() like that. It should be a single string with one placeholder for the directory.