Ensure all validation results use translatable strings except when created from throwables

in practice, but also string[]--especially in tests.

I think maybe it is only in tests. We should fix that rather than updating the comments for bad practice. All the user facing errors and warning should be tranlable.

I have created another MR to fix the occurences

I have started it with 2 examples but someone else can take this over

Also regarding the overhead this would create I checked core and t() is used all the time in core. Since this module is headed for core I think it is fine to do this.

Actually so I could be wrong

In \Drupal\automatic_updates\Validator\StagedProjectsValidator::validateStagedProjects() we actually do

catch (\Throwable $e) {
      $event->addError([
        $e->getMessage(),
      ]);
      return;
    }

so this is non-test code that adds uses a non-translatable string. and this will be passed on to `\Drupal\package_manager\ValidationResult::createError`

If it is only the case that we use non-translated string if we are passing on exception messages we could add

PreOperationStageEvent::addException(\Exception $e, $summary = NULL);

and
ValidationResult::createErrorFromException(\Exception $e, $summary = NULL)

so can more clearer that when you are not just passing on an exception message you need to provide translatable strings.

Shouldn't this be tagged core-mvp? AFAIK translatability is one of the core gates: https://www.drupal.org/about/core/policies/core-change-policies/core-gates (although I don't see it listed? 🤯)

The test \Drupal\Tests\package_manager\Kernel\StagedDBUpdateValidatorTest fails now if I try to use PluralTranslatableMarkup in \Drupal\package_manager\Validator\StagedDBUpdateValidator
but If I don't use PluralTranslatableMarkup and directly assign value then tests pass .

To figure out what's going on in #18: put a breakpoint in Connection::__sleep() and observe what is calling it.

So, actually problem is that if I have xdebug on and I don't even use any translation methods than all tests are failing with same error.

#3320782: xdebug being enabled causes tests to fail without clear indication that it is the problem just was a massive distraction because merely enabling xdebug caused every test method in that class to fail! Bumped that to Critical in #3320782-7: xdebug being enabled causes tests to fail without clear indication that it is the problem.

Taking this over because after ~45 mins of debugging we still hadn't found the root cause. This is definitely very tricky!

Huh, even with #3325522: Automatic Updates & Package Manager should use DependencySerializationTrait when needed applied, the Connection::__sleep() exception is thrown.

I spent hours today on debugging this. I even debugged PHPUnit step-by-step. It must be related to some global state because

  protected $backupGlobals = FALSE;
  protected $runTestInSeparateProcess = FALSE;
  protected $backupStaticAttributes = FALSE;

makes it fail in a different way.

If I add an early return []; to Connection::__sleep(), it'll instead try to serialize ContainerBuilder, and if I do the same thing there, it's a Closure… 😬😬😬😬

🫣 I had to resort to modifying vendor/phpunit/phpunit/src/Util/PHP/Template/TestCaseMethod.tpl to make me able to debug __phpunit_run_isolated_test(), where the

    print serialize(
      [
        'testResult'    => $test->getResult(),
        'numAssertions' => $test->getNumAssertions(),
        'result'        => $result,
        'output'        => $output
      ]
    );

call seems to have been causing this problem.

→ this means that some test is failing and $result contains a failed PHPUnit assertion, which is why PHPUnit will try to generate helpful error output for it, including the stack trace. That stack trace in turn at some point contains a DB connection …

To prove this, I had to painstakingly debug phpunit's template-based test runner logic (see start of this comment), which eventually led me to … Stage. If I add public function __sleep() { return []; } to it, I get a helpful error output instead of the virtually impossible to debug error message that @kunal.sachdev showed in #18:

Time: 00:03.869, Memory: 10.00 MB

There were 2 failures:

1) Drupal\Tests\package_manager\Kernel\StagedDBUpdateValidatorTest::testFileDeleted with data set "hook_update_N" ('install')
Failed asserting that two arrays are identical.
--- Expected
+++ Actual
@@ @@
     0 => Array &1 (
         'severity' => 1
         'messages' => Array &2 (
-            0 => 'System'
-            1 => 'Stark'
+            0 => 'System (1 update)'
+            1 => 'Stark (1 update)'
         )
         'summary' => 'Possible database updates have been detected in the following extensions.'
     )
 )

/Users/wim.leers/core/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:122
/Users/wim.leers/core/vendor/phpunit/phpunit/src/Framework/Constraint/IsIdentical.php:79
/Users/wim.leers/core/modules/contrib/automatic_updates/package_manager/tests/src/Traits/ValidationTestTrait.php:31
/Users/wim.leers/core/modules/contrib/automatic_updates/package_manager/tests/src/Kernel/PackageManagerKernelTestBase.php:200
/Users/wim.leers/core/modules/contrib/automatic_updates/package_manager/tests/src/Kernel/StagedDBUpdateValidatorTest.php:109
/Users/wim.leers/core/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

[…]

Where exactly is that DB connection then? Glad you asked. That too took a lot of time to understand. Heck, what's worse, even if you modify Connection::__sleep() to just return the empty array, there's another! Then it's this dependency chain: Stage → file_system (FileSystem) → container.channel.file (LoggerChannel) → current_user (AccountProxy) → event_dispatcher (ContainerAwareEventDispatcher) → container. If you fix that one, you're faced with a serialized Closure, which … PHP simply does not support.

So, in a way, \Drupal\Core\File\FileSystem is to blame. And many others. But …

⚠️ The reason that this was "never before" a problem: #3307611: Create a validator to add a warning if updated extensions have database updates added Stage $stage as a parameter to \Drupal\Tests\package_manager\Kernel\PackageManagerKernelTestBase::assertStatusCheckResults(), and we simply haven't had to debug this before.

Let's make sure not a single other soul on this project ever has to spend another minute on this nightmare: https://git.drupalcode.org/project/automatic_updates/-/merge_requests/54...

This means @kunal.sachdev (or somebody else) can now continue this issue, because now there are sensible test failures:

Testing Drupal\Tests\package_manager\Kernel\StagedDBUpdateValidatorTest
.FFFFFF.                                                            8 / 8 (100%)

Time: 00:15.938, Memory: 6.00 MB

There were 6 failures:

1) Drupal\Tests\package_manager\Kernel\StagedDBUpdateValidatorTest::testFileDeleted with data set "hook_update_N" ('install')
Failed asserting that two arrays are identical.
--- Expected
+++ Actual
@@ @@
     0 => Array &1 (
         'severity' => 1
         'messages' => Array &2 (
-            0 => 'System'
-            1 => 'Stark'
+            0 => 'System (1 update)'
+            1 => 'Stark (1 update)'
         )
         'summary' => 'Possible database updates have been detected in the following extensions.'
     )
 )
[…]

🥳

Needs works for 1 point about an unrelated(I think) change

Agreed with @tedbow's concerns. We need to revert the precision in the messages and the function signatures, because it's not accurate. I think it was introduced to be able to meaningfully use PluralTranslatableMarkup, but until #3253828: Use static analysis to detect new update functions, to reduce false positives in StagedDBUpdateValidator lands, that is premature. 😅

Needs work for a bunch of incorrect formatting/indentation cases 😅

@omkar.podey: you only fixed the 2 formatting problems that I provided suggestions for in https://git.drupalcode.org/project/automatic_updates/-/merge_requests/54.... There's 4 that I did not provide suggestions for, that still need to be fixed 😅

Found some problems with the use of the $summary parameter on \Drupal\package_manager\Event\PreOperationStageEvent::addError(), but that's a pre-existing problem. Tagging Needs followup but that's for @tedbow to decide/sort out.

Needs work for 7.4 failures. Still reviewing

@tedbow: Did you forget to mark this as needing review?

looks good to me

Assigning to myself to commit when tests pass

Thanks everyone 💪🎉!