A simple response object for methods which need to return a true/false *and* a message

Aye... Captain fubhy wants that, YarrrrRRR

The problem still exists, as you can see in the tmgmt_microsoft module. The functionality you describe is very important and much required as general solution! For example see #2009946: Microsoft translator can not translate from English to * but the problem also exists with other translation plugins!

Oh yeah, that's a major 8.x thingy because we want to have clean interfaces.

Maybe something like this will do it.

1. +++ b/src/TMGMTResponse.php
   @@ -0,0 +1,51 @@
   +class TMGMTResponse{
   

   One space after class name.

2. +++ b/src/TMGMTResponse.php
   @@ -0,0 +1,51 @@
   +  /**
   +   * @param bool $bitwise
   

   A comment like: "Constructs a new TMGMTResponse object."

3. +++ b/src/TMGMTResponse.php
   @@ -0,0 +1,51 @@
   +  function __construct($bitwise, $message = "", $data = array()) {
   +    $this->bit = $bitwise;
   

   public access modifier is missing.

   Why not use the same variable name ($bit) as a parameter too?

+++ b/src/TMGMTResponse.php
@@ -0,0 +1,51 @@
+   * @param bool $bitwise
...
+  function __construct($bitwise, $message = "", $data = array()) {

$bitwise should be named $success

Also $data is typically named different:
SafeMarkup::format and also StringTranslation name it $args
(Monitoring::addMessage() names it $variables)
I'd say we follow core naming.

Also, next step is using the class in the methods Berdir outlined.
While working on this issue here, you should checkout all current (google, microsoft, gengo) translators and create tickets there... And create a similar issue that switches to the new implementation. We want to commit this API change in sync on all current projects, when we see all fixes combined pass locally.

Applied, creating followups subsequently.

+++ b/src/TMGMTResponse.php
@@ -0,0 +1,53 @@
+   * True or False for response.
...
+   * Message in case bit is false.
...
+   *   Response either TRUE or FALSE.

TRUE true or True?

You can not put using this response object in a followup. It needs to be used somehow with this patch and test covered.

Added test coverage and implemented object instead of getNotAvailableReason() method.

+++ b/src/Form/JobForm.php
@@ -422,7 +423,8 @@ class JobForm extends TmgmtFormBase {
       if (!$translator->isAvailable()) {
...
+        $result = new TMGMTResponse(FALSE, t('@translator is not available. Make sure it is properly !configured.'), array('@translator' => $translator->label(), '!configured' => $translator->link(t('configured'))));
+        $form_state->setErrorByName('translator', $result->getMessage());

I think isAvailable should return the $result with a proper message. The form should just extract the text and display it as error.

Uploading changes so far, changed the isAvailable, working on canTranslate().

Some changes for canTRanslate still some work needed.

This is working so far, need to apply the 2. suggestion.

1. +++ b/src/TranslatorPluginBase.php
   @@ -49,12 +49,21 @@ abstract class TranslatorPluginBase extends PluginBase implements TranslatorPlug
   +    if ($job->getTranslator()) {
   +      return new TMGMTResponse(FALSE, t('@translator can not translate from @source to @target.', array(
   +        '@translator' => $job->getTranslator()->label(),
   +        '@source' => $job->getSourceLanguage()->getName(),
   +        '@target' => $job->getTargetLanguage()->getName()
   

   We have a $translator in this context. Why get back to the $job?

2. +++ b/src/TranslatorPluginBase.php
   @@ -49,12 +49,21 @@ abstract class TranslatorPluginBase extends PluginBase implements TranslatorPlug
   +      return new TMGMTResponse(FALSE, t('@translator can not translate.'));
   

   What @translator? This case should not exist.

3. +++ b/src/TranslatorPluginInterface.php
   @@ -22,23 +22,12 @@ interface TranslatorPluginInterface extends PluginInspectionInterface {
       *   TRUE if the translator plugin is available, FALSE otherwise.
   

   Fix the desc.

4. +++ b/src/TranslatorPluginInterface.php
   @@ -22,23 +22,12 @@ interface TranslatorPluginInterface extends PluginInspectionInterface {
   +
   

   Too much space.

5. +++ b/tmgmt_test/src/Plugin/tmgmt/Translator/TestTranslator.php
   @@ -99,7 +100,11 @@ class TestTranslator extends TranslatorPluginBase implements TranslatorRejectDat
   +        '@translator' => $job->getTranslator()->label(),
   

   $translator->label();

okay this should solve that.

+++ b/src/Entity/Job.php
@@ -494,10 +495,10 @@ class Job extends ContentEntityBase implements EntityOwnerInterface, JobInterfac
+    return new TMGMTResponse(FALSE, t('Translation cant be requested'));

+++ b/src/Entity/Translator.php
@@ -299,7 +300,7 @@ class Translator extends ConfigEntityBase implements TranslatorInterface {
+    return new TMGMTResponse(FALSE);

+++ b/src/TranslatorPluginInterface.php
@@ -15,30 +15,19 @@ use Drupal\Component\Plugin\PluginInspectionInterface;
-
...
+

I think that you should add a message with this FALSE response.

Should be can not or can't, but not cant

Should only be one emply line between the functions.

Should not delete this empty line.

Now it looks fine for me.

1. +++ b/src/Tests/TMGMTUiTest.php
   @@ -289,6 +289,13 @@ class TMGMTUiTest extends TMGMTTestBase {
   +    $this->drupalGet(t('admin/tmgmt/jobs/@jobs', array('@jobs' => $job->id())));
   

   t() marks the string as translatable and is meant for translatable strings, what you want is SafeMarkup::format() [edit:] actually, that marks the string safe for displaying, still too much. You actually just want to concatenate the job id at the of the path:
   $this->drupalGet('admin/tmgmt/jobs/' . $job->id());

2. +++ b/src/Tests/TMGMTUiTest.php
   @@ -289,6 +289,13 @@ class TMGMTUiTest extends TMGMTTestBase {
   +    $this->assertUniqueText(t('1 error has been found:'));
   +
   

   Why not assert the field error itself?

   Test translator (auto created) can not translate from English to German.

Okay, fixed.

1. +++ b/src/Entity/Translator.php
   @@ -309,7 +310,11 @@ class Translator extends ConfigEntityBase implements TranslatorInterface {
   +      '!configured' => $this->link(t('configured')
   +      )
   

   Move the bracket up.

2. +++ b/src/TMGMTResponse.php
   @@ -0,0 +1,65 @@
   +   * Message in case bit is FALSE.
   

   nitpick: it's not bit anymore

After that I'll test a little bit and then set it to RTBC.

Done.

1. +++ b/src/TranslatorPluginInterface.php
   @@ -14,31 +14,20 @@ use Drupal\Component\Plugin\PluginInspectionInterface;
     */
   -interface TranslatorPluginInterface extends PluginInspectionInterface {
    
   +interface TranslatorPluginInterface extends PluginInspectionInterface {
      /**
   

   /**
    * CLASS DESCRIPTION
    *
    * @ingroup GROUP
    */
   interface ISample extends IBase {
     
     /**
      * METHOD DESCRIPTION
   ...

   I wonder why you ever changed that.

2. +++ b/tmgmt_test/src/Plugin/tmgmt/Translator/TestTranslator.php
   @@ -99,7 +100,11 @@ class TestTranslator extends TranslatorPluginBase implements TranslatorRejectDat
   +        '@target' => $job->getTargetLanguage()->getName()
   

   Missing comma.

okay this may do that as well.

+++ b/src/TranslatorPluginInterface.php
@@ -14,6 +14,7 @@ use Drupal\Component\Plugin\PluginInspectionInterface;
+

Still too much... The line you added in patch #21 was wrong, basically revert that change, have the format just like the coding standards / take a look at the Code Sniffer errors.

Okay.

Created followups for the translator plugins, added them to the issue overview.

Looks good now.

Bumping this issue.

Note that the issue #2543564: Rename $controller to $plugin with recent activity will break previous patches.

Some work still needed, feedback apreciated.

Some small changes based on suggestions.
Edit: that interdiff is totally wrong.

+++ b/src/Translator/AvailableResult.php
@@ -0,0 +1,40 @@
+use Drupal\tmgmt\TranslatorInterface;
+
+/**
+ * Class AvailableResult
+ *
+ * @package Drupal\tmgmt\Translator

Add description of the class and the methods.

+++ b/src/Translator/TranslatableResult.php
@@ -0,0 +1,55 @@
+use Drupal\tmgmt\JobInterface;
+
+/**
+ * Class AvailableResult
+ *
+ * @package Drupal\tmgmt\Translator

Same for these class

+++ b/src/Translator/TranslatorResult.php
@@ -0,0 +1,60 @@
+namespace Drupal\tmgmt\Translator;
+
+/**
+ * TMGMT Translator Result abstract class.
+ */
+abstract class TranslatorResult {

Extend description also here.

Clarified the summary a bit.

Rerolled

Okay maybe something like this?

Some nitpicks, otherwise looks good to me.

1. +++ b/src/Entity/Job.php
   @@ -517,11 +518,11 @@ class Job extends ContentEntityBase implements EntityOwnerInterface, JobInterfac
   +    return TranslatableResult::no(t('Translation can not be requested'));
   

   Missing a dot.

2. +++ b/src/Translator/AvailableResult.php
   @@ -0,0 +1,37 @@
   + * Contains Drupal\tmgmt\Translator\AvailableResult
   

   Same as 1

3. +++ b/src/Translator/AvailableResult.php
   @@ -0,0 +1,37 @@
   +
   

   Extra space

4. +++ b/src/Translator/TranslatableResult.php
   @@ -0,0 +1,37 @@
   +
   

   Same as 3

:-)

I really don't get why everyone is trying to refactor the whole architecture (and in a different way than what was defined).

The only intention was to introduce the response object and make the methods like $translator->canTranslate() or $translator->isAvailable() return the object instead of TRUE/FALSE. Sascha clearly stated that the only methods that should disappear are getNotAvailableReason and getNotCanTranslateReason.

The response object needs to be an instance as a result of the method. There are no static calls.
The only thing the response object should provide is a factory to simplify construction.
All these static, abstract and derived classes look like a mismatch in abstraction and limiting flexibility: A translator can no more implement an own logic.

Not sure if it makes sense to proceed with the current setup. Possibly Sascha or i should provide the implementation.

This should be the procedure then, up to this point the isAvailable and canTranslate haven't been renamed yet. Will do if following the right track.

sorry forgot to make a call to a method.

Yeah we are kindof back on track... :-)

1. +++ b/src/Form/JobForm.php
   @@ -421,11 +423,11 @@ class JobForm extends TmgmtFormBase {
   +      if (!($translator->isAvailable()->getSuccess())) {
   +        $form_state->setErrorByName('translator', $translator->isAvailable()->getMessage());
   

   Do not run the check twice. Treat the method like you get back a complex result. Run it first $translator->isAvailable() and then persist it in a var. Same applies for all subsequent cases also canTranslate().

2. +++ b/src/Form/JobForm.php
   @@ -498,11 +500,12 @@ class JobForm extends TmgmtFormBase {
        if (!$translator->isAvailable()) {
   

   Accidentally unchanged. Search for all calls and fix them! :-)

3. +++ b/src/Translator/AvailableResult.php
   @@ -0,0 +1,40 @@
   +  public static function yes() {
   ...
   +  public static function no($message) {
   
   +++ b/src/Translator/TranslatableResult.php
   @@ -0,0 +1,44 @@
   +  public static function yes() {
   ...
   +  public static function no($message) {
   

   These static methods should be the only static ones that exist on TranslatorResult and should do:
   $result = new static();

   The TranslatableResult and AvailableResult are just class definitions without any method.

4. +++ b/src/Translator/TranslatorResult.php
   @@ -0,0 +1,81 @@
   +  protected static $success;
   ...
   +  protected static $message;
   ...
   +  public static function getMessage() {
   ...
   +  public static function getSuccess() {
   ...
   +  protected static function setNo($message) {
   ...
   +  protected static function setYes() {
   ...
   +  public static function yes() {
   ...
   +  public static function no($message) {
   

   I don't see why these should be static. The response message and success needs to be inside the instance.

Fixed that, now i can edit the methods name. Any suggestion?

The latest patch is wrongly created, it's missing some of the latest changes on the main branch.

On the way to the solution. :-)

1. +++ b/src/Translator/TranslatorResult.php
   @@ -37,7 +37,7 @@
        return self::$success;
   

   return $this->success;
   And all others similar.

2. +++ b/src/Form/TranslatorForm.php
   @@ -143,7 +143,7 @@ class TranslatorForm extends EntityForm {
   -          'callback' => array($this, 'ajaxTranslatorPluginSelect'),
   +          'callback' => array($this, 'ajaxTranslaorPluginSelect'),
   

   Please rebase! You are reverting fixed things!

Okay yeah sorry about the rebase, this should be it. :)

Some test fails are from #2579395: Test fails due to recent core change with FormattableString

1. +++ b/src/Form/JobForm.php
   @@ -497,12 +501,13 @@ class JobForm extends TmgmtFormBase {
   +    if (!$translator->checkAvailable()->getSuccess()) {
   +      $result = AvailableResult::no(t('@translator is not available. Make sure it is properly :configured.', array('@translator' => $translator->label(), ':configured' => $translator->link(t('configured')))));
   +      $form['#description'] = Xss::filter($result->getMessage());
   

   No, checkAvailable() provides the result containing the message...

2. +++ b/src/Form/JobForm.php
   @@ -497,12 +501,13 @@ class JobForm extends TmgmtFormBase {
   +    elseif ($job->getTargetLangcode() && !$translator->checkTranslatable($job)->getSuccess()) {
   +      $form['#description'] = Xss::filter($job->getTranslator()->checkTranslatable($job)->getMessage());
   

   You still call checkTranslatable() twice.

After reviewing with a different translator seems like we missed the auto-escape and the return message of the translatorbase is wrong. When a translator is not available it should be configured, not shown that the target language of translation is not available.

This patch adds test coverage as well as the fix and the implementation for not_available in test translator.