Problem/Motivation

AS a user that works with feeds
WHEN I import a feed
THEN I should not get an error or generate a error in the log.

The existing code does not seem to take into account that these may be translatable strings instead of just plain strings.

Steps to reproduce

  • Drupal Core 8.9.13
  • Import a feed
  • Expected response: Feed imports and but errors during reporting.
  • Actual response:
    • Browser:
      An AJAX HTTP error occurred.
HTTP Result Code: 500
Debugging information follows.
Path: /batch?id=17950&op=do_nojs&op=do
StatusText: Internal Server Error
ResponseText: The website encountered an unexpected error. Please try again later.
    • Log:
      InvalidArgumentException: $string ("There are no new @items.") must be a string. in Drupal\Core\StringTranslation\TranslatableMarkup->__construct() (line 132 of /var/www/drupal/app/docroot/core/lib/Drupal/Core/StringTranslation/TranslatableMarkup.php).

Proposed resolution

To minimize scope, created the attached patch for Feeds instead of Drupal Core.

Remaining tasks

Incorporate review and consider incorporation into Feeds 8.x-3.0.

User interface changes

N/A

API changes

N/A

Data model changes

N/A

CommentFileSizeAuthor
#3 3199207.patch2.76 KBkwiechmann
#2 3199207.patch2.82 KBkwiechmann

Comments

kwiechmann created an issue. See original summary.

kwiechmann’s picture

kwiechmann commented
StatusFileSize
new 3199207.patch2.82 KB
kwiechmann’s picture

kwiechmann commented
StatusFileSize
new 3199207.patch2.76 KB

Updated patch file.

megachriz’s picture

megachriz
Primary language Dutch
commented

I don't have seen this error before. Can you post the full backtrace of the error?

I see that the result of formatPlural() seems to be a PluralTranslatableMarkup instance and the result of t() a TranslatableMarkup instance, according to \Drupal\Core\StringTranslation\StringTranslationTrait. And that \Drupal\Core\StringTranslation\TranslatableMarkup::__construct() doesn't accept instances of TranslatableMarkup. But I don't understand why none of the automated tests in Feeds fail on this.

Ideally, we would have an automated test that demonstrates the bug.

megachriz’s picture

megachriz
Primary language Dutch
commented
Status: Active » Postponed (maintainer needs more info)
kwiechmann’s picture

kwiechmann commented
Status: Postponed (maintainer needs more info) » Active

@MegaChriz Thanks for responding. Here is the stack output from Xdebug:

  1. TranslatableMarkup.php:134, Drupal\Core\StringTranslation\TranslatableMarkup->__construct()
  2. bootstrap.inc:356, t()
  3. Stdout.php:54, Drupal\log_stdout\Logger\Stdout->log()
  4. LoggerChannel.php:127, Drupal\Core\Logger\LoggerChannel->log()
  5. State.php:141, Drupal\feeds\State->logMessages()
  6. Feed.php:310, Drupal\feeds\Entity\Feed->finishImport()
  7. FeedsExecutable.php:320, Drupal\feeds\FeedsBatchExecutable->finish()
  8. FeedsExecutable.php:118, Drupal\feeds\FeedsBatchExecutable->processItem()
  9. batch.inc:295, _batch_process()
  10. batch.inc:137, _batch_do()
  11. batch.inc:93, _batch_page()
  12. BatchController.php:55, Drupal\system\Controller\BatchController->batchPage()
  13. EarlyRenderingControllerWrapperSubscriber.php:123, call_user_func_array:{/var/www/docroot/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php:123}()
  14. EarlyRenderingControllerWrapperSubscriber.php:123, Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure:/var/www/docroot/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php:121-124}()
  15. Renderer.php:573, Drupal\Core\Render\Renderer->executeInRenderContext()
  16. EarlyRenderingControllerWrapperSubscriber.php:124, Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext()
  17. EarlyRenderingControllerWrapperSubscriber.php:97, Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure:/var/www/docroot/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php:96-98}()
  18. HttpKernel.php:151, Symfony\Component\HttpKernel\HttpKernel->handleRaw()
  19. HttpKernel.php:68, Symfony\Component\HttpKernel\HttpKernel->handle()
  20. Session.php:57, Drupal\Core\StackMiddleware\Session->handle()
  21. KernelPreHandle.php:47, Drupal\Core\StackMiddleware\KernelPreHandle->handle()
  22. PageCache.php:106, Drupal\page_cache\StackMiddleware\PageCache->pass()
  23. PageCache.php:85, Drupal\page_cache\StackMiddleware\PageCache->handle()
  24. Cors.php:60, Asm89\Stack\Cors->handle()
  25. ReverseProxyMiddleware.php:47, Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle()
  26. NegotiationMiddleware.php:52, Drupal\Core\StackMiddleware\NegotiationMiddleware->handle()
  27. StackedHttpKernel.php:23, Stack\StackedHttpKernel->handle()
  28. DrupalKernel.php:708, Drupal\Core\DrupalKernel->handle()
  29. index.php:19, {main}()
megachriz’s picture

megachriz
Primary language Dutch
commented

Interesting, thanks for posting the backtrace. I see that for that log_stdout a similar issue exists:
#3065639: Message translated twice causes error in Migrate UI.

So it could as well be an issue for that module. Not completely sure though. Maybe Feeds shouldn't pass TranslatableMarkup objects to the logger? In that case, I think the adjustment needs to be done in \Drupal\feeds\State\logMessages().

megachriz’s picture

megachriz
Primary language Dutch
commented

Hm, Psr\Log\LoggerInterface does allow the message to be an object, if it implements __toString(). TranslatableMarkup does implement that method via the trait \Drupal\Component\Utility\ToStringTrait.

kwiechmann’s picture

kwiechmann commented

Based on your last comment, it appears that this might be a Drupal Core bug? Figured I would be best to start at the module before moving towards Core.

Here is what I am thinking for an update to docroot/core/lib/Drupal/Core/StringTranslation/TranslatableMarkup.php:

  public function __construct($string, array $arguments = [], array $options = [], TranslationInterface $string_translation = NULL) {
    // Original: if (!is_string($string)) {
    if (!is_string($string) && !method_exists($string, '__toString')) {
      $message = $string instanceof TranslatableMarkup ? '$string ("' . $string->getUntranslatedString() . '") must be a string.' : '$string ("' . (string) $string . '") must be a string.';
      throw new \InvalidArgumentException($message);
    }
    parent::__construct($string, $arguments);
    $this->options = $options;
    $this->stringTranslation = $string_translation;
  }
megachriz’s picture

megachriz
Primary language Dutch
commented

No, with the comment in #8 I meant that it is allowed to pass objects to the logger. In #7 I asked the question if only strings may be passed to the logger:

Maybe Feeds shouldn't pass TranslatableMarkup objects to the logger?

Answer: passing an object to a logger is fine as long as it implements __toString().

So I think the bug is in log_stdout, which passes the log message it receives straight to t(), without checking if the message is a string. The bug looks like to be already fixed in the dev version of that module, as I no longer could spot a call to t() there.

So maybe close this issue as "works as designed", if you agree?

kwiechmann’s picture

kwiechmann commented
Status: Active » Closed (works as designed)

Thanks for your assistance. The patch against feeds works and meets my current needs. Not sure when I will be able to return to this issue but will look into patching log_stdout 8.x-1.1 or upgrading to log_stdout 8.x-2.x-dev (which has few sites).

Closing as works as designed.