Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.\Drupal\user\Tests\UserRegistrationTest::testUuidFormState() has the following line (in 2 places):
$this->assertTrue($user_storage->loadByProperties(['name' => $edit['name']]));
The aim is to check whether a user was successfully loaded with a given name. assertTrue() then does:
protected function assertTrue($value, $message = '', $group = 'Other') {
return $this->assert((bool) $value, $message ? $message : SafeMarkup::format('Value @value is TRUE.', array('@value' => var_export($value, TRUE))), $group);
}
Given that $value is an array of user objects this not only creates a fairly nasty message for the assertion, but also risks circular reference errors.
Specifically, we have used dependency injection to swap out the entity class and we then perform some action in ::postLoad() which reads from a base field on the entity. This causes it to load the field definitions and set ::$typedData, which contains ::$entity - a recursive reference. The var_export() in assertTrue() then uses a huge amount of memory before eventually fatally failing with this trace:
var_export does not handle circular references
var_export(Array, 1)
Drupal\simpletest\TestBase->assertTrue(Array)
Drupal\user\Tests\UserRegistrationTest->testUuidFormState()
Drupal\simpletest\TestBase->run()
_simpletest_batch_operation(Array, '4', Array)
call_user_func_array('_simpletest_batch_operation', Array)
_batch_process()
_batch_do()
_batch_page(Object)
Drupal\system\Controller\BatchController->batchPage(Object)
call_user_func_array(Array, Array)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}()
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}()
call_user_func_array(Object, Array)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1)
Stack\StackedHttpKernel->handle(Object, 1, 1)
Drupal\Core\DrupalKernel->handle(Object)| Comment | File | Size | Author |
|---|---|---|---|
| #9 | 2632662-9-pass_message_remove_SafeMarkup.patch | 2.35 KB | andrewbelcher |
| #9 | 2632662-9-pass_message.patch | 1.15 KB | andrewbelcher |
Comments
Comment #2
dawehnerYeah we should totally do assertTrue($object !== FALSE) instead or even fix assertTrue to not show objects?
Comment #3
andrewbelcher CreditAttribution: andrewbelcher at FreelyGive commentedHere's a patch I've written that solves it (for this specific case). @dawehner's suggestion of fixing it in
assertTrue()would mean it also solves it for other potential cases, but this one solves it simply by providing$messageand therefore avoiding thevar_export()(and also making the message nicer).I'm making use of
SafeMarkup::format()as that is what is inassertTrue(), though I'm aware it's deprecated. I wasn't sure what I was supposed to use instead, but if someone wants to point me in the right direction I'm very happy to update the patch.I don't know if we need test coverage for this, it depends whether using
assertTrue()for this kind of case is really supported? If it's needed, let me know where it should live and I can have a bash.As it's an array, the other option is to do either
assertTrue(!empty(...))orassertTrue(count(...)). Again, happy to re-do patch if an alternative solution is preferred.Comment #4
yanniboi CreditAttribution: yanniboi at FreelyGive commentedLike @andrewbelcher said, fixing assertTrue() would probably be beneficial, however also more risky.
The patch works for me and is fairly non-intrusive, so I'm marking this RTBC. Feel free to change if anyone disagrees.
Comment #6
andrewbelcher CreditAttribution: andrewbelcher at FreelyGive commentedNot sure why that failed having passed multiple times before, try it again...
Comment #7
andrewbelcher CreditAttribution: andrewbelcher at FreelyGive commentedAs it passed...
Comment #8
alexpottre #3
So we should be using FormattableMarkup instead. Thanks.
Comment #9
andrewbelcher CreditAttribution: andrewbelcher at FreelyGive commentedI've attached to patches. One just replaces
SafeMarkupwithFormattableMarkupwhere I'm making changes. The other replaces all uses.Comment #18
quietone CreditAttribution: quietone as a volunteer commentedThe use of assertTrue in UserRegistrationTest was changed/fixed in #2742585: Deprecate dangerous assertTrue/False() compatibility overrides in PHPUnit Kernel, Functional and FunctionalJavascript tests. The patch also changes two assertRaw, and those changes have already occurred in #2958429: Properly deprecate SafeMarkup::format().
Therefore, closing as outdated. If this is incorrect reopen the issue, by setting the status to 'Active', and add a comment explaining what still needs to be done.
Thanks!