Skipped translation import shows dblog link message that is not yet available in installer

Here's a screenshot of the said installer message.

The message comes from locale_translate_batch_finished() in core/modules/locale/locale.bulk.inc which is used elsewhere as well:

if (\Drupal::moduleHandler()->moduleExists('dblog')) {
  $message = format_plural($skips, 'One translation string was skipped because of disallowed or malformed HTML. <a href="@url">See the log</a> for details.', '@count translation strings were skipped because of disallowed or malformed HTML. <a href="@url">See the log</a> for details.', array('@url' => url('admin/reports/dblog')));
        }

I'm not sure if there is a decent way to detect when it's run in the install context though?

Tagged so it gets noticed by the right people.

Fixing tags. Also the condition already includes a check for dblog module existing. So the link is only printed then, right? At the point this is printed, modules are already enabled, so you could follow the link. *Except* you are not logged in, which is a regression in Drupal 8, no? You used to be logged in at this point in the installer, no?

At the point this message is printed the dblog module exists but the user is not yet logged in and therefore able to follow the link to see the details. This is the same screen one would set up site config and user 1.

In D7 the details are printed to the screen instead of dblog. I also believe you are not logged in at that point yet since this is before setting up the user account/site config.

Assuming that the user should not be logged in at this point in D8, the correct solution should be either outputting the message directly or updating the copy without the link? IMHO the user should know something didn't work quite right but there's no way to find out except to finish the site install.

All right, that makes sense :) Indeed our best is probably we should check for installer constants / "is maintenance mode".

Attached patch checks if the batch is run in the maintenance mode, specifically if it is run as part of the installer.
To test run the installer in Estonian ("Eesti" in the language selection). There's one string that has an issue and I have not yet fixed in order to be able to replicate this bug in the installer.

The fix looks fine :) As good as the (ugly) installer stuff can get :)

This is not clear to me: "There's one string that has an issue and I have not yet fixed in order to be able to replicate this bug in the installer." Can you elaborate?

There's a translation in Estonian that contains disallowed/malformed HTML which will cause the message to pop up in the installer in the first place. I administer the Estonian language translation, therefore I could go and fix it but for the sake of this issue I have not yet done it. Does that explain it?
I don't know if this link will work for you but this is the string causing the issue - https://localize.drupal.org/translate/languages/et/translate?project=&st.... The original string is <No value>

Right, makes sense, looks good :)

Can we add a test for this? It seems like protection against malformed translations should be something we'd want to catch, regardless of where the problem lies.

The core/update.php update_results_page() function contains a same use case and solution.

 // Report end result.
  if (\Drupal::moduleHandler()->moduleExists('dblog') && user_access('access site reports')) {
    $log_message = ' All errors have been <a href="' . base_path() . '?q=admin/reports/dblog">logged</a>.';
  }
  else {
    $log_message = ' All errors have been logged.';
  }

I rerolled the patch see comment above.

I will add a test soon.

Here it is a patch with tests and a little typo (One files -> One file)

- // The import should have created 0 string and rejected 0.
It's a bit strange that we remove this comment, but there's no replacement. What does that assertion test then?

Also, let's use \Drupal\Core\Session\AccountInterface::hasPermission() , as i see there's no reason to use the deprecated style.
That's what i see so far.

@Aron Novak Thanks your review

1. I corrected the patch. Change user_access() to \Drupal::currentUser()->hasPermission().
2. It is an ugly copy/pasted comment from above. This assertion tells you the empty file not imported. If you want it, I put it back. :)

pp

Rerolled.

Adapted to code style in HEAD.

Looks good. One minor comment/question

+++ b/core/modules/locale/src/Tests/LocaleImportFunctionalTest.php
@@ -79,14 +86,33 @@ public function testStandalonePoFile() {
+    $this->assertRaw(t('One translation file could not be imported. See the log for details.'), 'The empty translation file was successfully imported.');

message says couldn't not be imported and assert says empty file imported successfully?

+1 to RTBC.

Corrected #25 and replicated a comment for clarifying assertion.

diff --git a/core/modules/locale/src/Tests/LocaleImportFunctionalTest.php b/core/modules/locale/src/Tests/LocaleImportFunctionalTest.php
index cd1575f..011e9c1 100644
<ol>

<li>
<code>
+++ b/core/modules/locale/src/Tests/LocaleImportFunctionalTest.php
@@ -30,6 +30,12 @@ class LocaleImportFunctionalTest extends WebTestBase {
   /**
+   * A user able to create languages, import translations and access site
+   * reports.
+   */
+  protected $adminUserAccessSiteReports = NULL;
+

No need for the = NULL and this needs an @var annotation.

+++ b/core/modules/locale/src/Tests/LocaleImportFunctionalTest.php
@@ -79,16 +86,35 @@ public function testStandalonePoFile() {
+    $this->drupalLogin($this->adminUserAccessSiteReports);
...
+    $this->drupalLogin($this->adminUserAccessSiteReports);

+++ b/core/modules/locale/src/Tests/LocaleImportFunctionalTest.php
@@ -79,16 +86,35 @@ public function testStandalonePoFile() {
 
+
+    $this->drupalLogin($this->adminUser);

Edit: removed incorrect comment

Handled feedback from #28.

an user => a user

Fixed the typo.

Looks good, resolves concerns raised.

Random db fails. Hope better this time :D

Committed 2a95441 and pushed to 8.0.x. Thanks!

Thanks all!

Minor tag cleanup - please ignore.