Refactor code where getMessagesSince() is used

I created a new function tmgmt_write_request_messages_for_jobItem().
Then I removed the duplicated code and called the right function.
I didn't remove the duplicated code in JobAbortForm(), because there is something that doesn't make any sense to me. the duplicated code is inside an if statement. But after the if statement, the function tmgmt_write_request_messages() gets called anyway.

I had to reroll the patch, This is the right one.

+++ b/src/Form/JobItemForm.php
@@ -558,7 +558,7 @@ class JobItemForm extends TmgmtFormBase {
+    tmgmt_write_request_messages_for_jobItem($form_state->getFormObject()->getEntity());

"tmgmt_write_request_messages_job_item" better?

+++ b/tmgmt.module
@@ -709,6 +709,31 @@ function tmgmt_write_request_messages(JobInterface $job) {
+    if ($text = $message->getType()) {

$message->getMessage() should work better.

Not sure if, following this approach, we will need to remove the original function at some point so we should add two new functions, one called "tmgmt_write_request_messages_job_item" and the other "tmgmt_write_request_messages_job"

Adjusted the names of the functions and fixed the tmgmt_write_request_messages_job function.
Also fixed a wrong call of the function above.

+++ b/src/Form/JobItemForm.php
@@ -558,7 +558,7 @@ class JobItemForm extends TmgmtFormBase {
-    tmgmt_write_request_messages($form_state->getFormObject()->getEntity()->getJob());
+    tmgmt_write_request_messages_job_item($form_state->getFormObject()->getEntity());

As you can see here it seems that the job is sent. Should we use instead tmgmt_write_request_job ?

tmgmt_write_request_job is being used now.

Accidentally had a typo in the source when I created the last interdiff. I already removed it in the last patch.

Good work, close enough.

+++ b/tmgmt.module
@@ -717,6 +717,31 @@ function tmgmt_write_request_messages(JobInterface $job) {
+ *    The translation job for which the message should be written.

Isn't this the job item?

1. +++ b/tmgmt.module
   @@ -699,7 +699,7 @@ function tmgmt_job_request_translation(JobInterface $job) {
     */
   -function tmgmt_write_request_messages(JobInterface $job) {
   +function tmgmt_write_request_messages_job(JobInterface $job) {
      $errors = FALSE;
   

   While this makes sense to clarify the function, this is an API change that's not really required.

   We're in RC phase, so we try to avoid API changes as much as possible.

   There's also a different way to solve this, see below.

2. +++ b/tmgmt.module
   @@ -717,6 +717,31 @@ function tmgmt_write_request_messages(JobInterface $job) {
   + */
   +function tmgmt_write_request_messages_job_item(JobItemInterface $job_item) {
   +  $errors = FALSE;
   +  foreach ($job_item->getJob()->getMessagesSince() as $message) {
   +    if ($message->getType() == 'debug') {
   

   JobItem actually has a getMessagesSince() method too, so we can use that directly.

   I'd think that's what I would expect to happen, since you otherwise get the messages for all items in the job, not just this item.

   Also, here's an idea. Instead of having two almost identical methods, we coud make one method that accepts both objects.

   The easiest way to do that is to just remove the type hint and document that both objects are accepted on the @param. The stricter way would be to make a new interface "MessagesInterfaces" and move getMessages() and getMessagesSince() to that, implementing it in both objects. Then we can type hint on that and could later on even use it for other objects (like local task (items) when those get messages too.

removed the tmgmt_write_request_messages_job_item function.
renamend tmgmt_write_request_messages_job to tmgmt_write_request_messages and adjusted the function to work for jobs and jobitems.

As mentioned in comment #3, what should I do about the duplicated code in the following case? The duplicated code is in an if statement, but the function gets called after the if statement anyways.

public function submitForm(array &$form, FormStateInterface $form_state) {
    /** @var \Drupal\tmgmt\Entity\Job $entity */
    $entity = $this->entity;
    if (!$entity->abortTranslation()) {
      // This is the case when a translator does not support the abort
      // operation.
      // It would make more sense to not display the button for the action,
      // however we do not know if the translator is able to abort a job until
      // we trigger the action.
      foreach ($entity->getMessagesSince() as $message) {
        /** @var \Drupal\tmgmt\MessageInterface $message */
        if ($message->getType() == 'debug') {
          continue;
        }
        if ($text = $message->getMessage()) {
          // We want to persist also the type therefore we will set the
          // messages directly and not return them.
          drupal_set_message($text, $message->getType());
        }
      }
    }
    else {
      $entity->addMessage('The user ordered aborting the Job through the UI.');
    }
    tmgmt_write_request_messages($entity);
    $form_state->setRedirectUrl($this->entity->toUrl());
  }

+++ b/src/Form/JobItemForm.php
@@ -558,7 +558,7 @@ class JobItemForm extends TmgmtFormBase {
     $render_data = NestedArray::getValue($form, $key);
-    tmgmt_write_request_messages_job($form_state->getFormObject()->getEntity()->getJob());
+    tmgmt_write_request_messages($form_state->getFormObject()->getEntity()->getJob());
     return $render_data;

we might want to pass the job item here, the current code might only get the job because the function requested that.

Removed the duplicated code in JobAbortForm.php and fixed the submitForm function.
Adjusted the ajaxReviewForm function in JobItemForm.php to call the tmgmt_write_request_messages with a job_item instead of a job as parameter.

+++ b/src/Form/JobAbortForm.php
@@ -37,29 +37,10 @@ class JobAbortForm extends ContentEntityConfirmFormBase {
-      // This is the case when a translator does not support the abort
-      // operation.
-      // It would make more sense to not display the button for the action,
-      // however we do not know if the translator is able to abort a job until
-      // we trigger the action.

hm. This is a useful comment I think. Lets keep that, move it above the if and try to reword it a bit so it makes sense not only if it fails.

I adjusted the comment.

Cool, just a nitpick:

+++ b/tmgmt.module
@@ -693,15 +693,15 @@ function tmgmt_job_request_translation(JobInterface $job) {
+ * @param \Drupal\tmgmt\JobInterface|\Drupal\tmgmt\JobItemInterface $job_or_job_item
...
+function tmgmt_write_request_messages($job_or_job_item) {
...
+  foreach ($job_or_job_item->getMessagesSince() as $message) {

Better to call it $entity instead of $job_or_job_item :)

+++ b/src/Form/JobItemForm.php
@@ -558,7 +558,7 @@ class JobItemForm extends TmgmtFormBase {
+    tmgmt_write_request_messages($form_state->getFormObject()->getEntity());

+++ b/tmgmt.module
@@ -813,7 +813,7 @@ function tmgmt_translation_review_form_update_state(array $form, FormStateInterf
+  tmgmt_write_request_messages($job_item);

Not fully sure why we want to pass a job item in these places (JobItemForm code mentioned in #15)...

Changed the name of the parameter from $job_or_job_item to $entity

Looks nice.

Thx, committed. :-)