Skip to content
Snippets Groups Projects

Draft: Deprecate ToStringTrait #3465827

5 unresolved threads

Closes #3465827

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
2 2
3 3 namespace Drupal\Component\Utility;
4 4
5 @trigger_error('The ' . __NAMESPACE__ . '\ToStringTrait is deprecated in drupal:11.1.0 and is removed from drupal:12.0.0. Use to implement the __toString() method instead. See https://www.drupal.org/node/3466031', E_USER_DEPRECATED);
6
5 7 /**
6 8 * Wraps __toString in a trait to avoid some fatal errors.
9 *
10 * @deprecated in drupal:11.1.0 and is removed from drupal:12.0.0. Use to
11 * implement the __toString() method instead.
  • 59 59 $string = 'May I have an exception?';
    60 60 $text = $this->getMockBuilder(TranslatableMarkup::class)
    61 61 ->setConstructorArgs([$string, [], [], $translation])
    62 ->onlyMethods(['_die'])
    63 62 ->getMock();
    64 $text
    65 ->expects($this->once())
    66 ->method('_die')
    67 ->willReturn('');
    • strange that this fails on the return type assertion, I would expect it throws an exception?

      We could drop the error handler and just expect an exception, but I'm wondering if we shouldn't just drop the whole test method. Because there is no code left anymore that we are testing with it, we'd just assert that our mock callback does indeed throw an exception. So I'd just remove the whole method and the errorHandler() method too.

    • Please register or sign in to reply
  • Andrey Postnikov added 16 commits

    added 16 commits

    Compare with previous version

  • Andrey Postnikov added 136 commits

    added 136 commits

    Compare with previous version

  • 73 throw new \Exception('Yes you may.');
    74 });
    75
    76 // We set a custom error handler because of https://github.com/sebastianbergmann/phpunit/issues/487
    77 set_error_handler([$this, 'errorHandler']);
    78 // We want this to trigger an error.
    79 (string) $text;
    80 restore_error_handler();
    33 ->method('render')
    34 ->willThrowException($exception);
    81 35
    82 $this->assertEquals(E_USER_ERROR, $this->lastErrorNumber);
    83 $this->assertMatchesRegularExpression('/Exception thrown while calling __toString on a .*MockObject_TranslatableMarkup_.* object in .*TranslatableMarkupTest.php on line [0-9]+: Yes you may./', $this->lastErrorMessage);
    36 $this->expectException(\Exception::class);
    37 $this->expectExceptionMessage($exception->getMessage());
    38 $this->assertTrue('' === (string) $text);
  • added 9 commits

    Compare with previous version

  • added 1 commit

    • 5c83efad - fix Component/Diff/DiffFormatter

    Compare with previous version

  • added 1 commit

    • 9bc2dd86 - replace usage with logger in Render/BigPipe to conform tests, @Message is not...

    Compare with previous version

  • 381 381 throw $e;
    382 382 }
    383 383 else {
    384 trigger_error($e, E_USER_ERROR);
    384 $this->logger->error($e->getMessage(), [
    385 'exception' => $e,
    386 'backtrace' => $e->getTraceAsString(),
    387 ]);
    • Comment on lines +384 to +387

      This change needs at least explanation on the issue. We're replacing an irrecoverable error with just a log message. It that right? It doesn't feel right.

    • Yes, it needs explanation why core allows to recover from this kind of error, the continue; after logger points that this error is supposed to return execution back!

      But I lack of idea why and this kind of logging has test coverage that watchdog entry has @message for some reason, asked Wim in Slack

    • It was added in the original big pipe issue and I don't think it's been discussed since. For me I would make this E_USER_WARNING + a follow-up to just allow the exception to be thrown.

    • Please register or sign in to reply
  • 615 621 throw $e;
    616 622 }
    617 623 else {
    618 trigger_error($e, E_USER_ERROR);
    624 $this->logger->error($e->getMessage(), [
    625 '@message' => $e->getMessage(),
  • Andrey Postnikov added 193 commits

    added 193 commits

    Compare with previous version

  • added 1 commit

    Compare with previous version

  • Please register or sign in to reply
    Loading