Problem/Motivation

The cast here does nothing: https://git.drupalcode.org/project/drupal/-/blob/00dfe3f40fe48e25a7b9a4c...
DateTimePlus::render() always returns a string (although it does not have a return type declaration).

It was introduced by #3548957: Deprecate ToStringTrait because the method was copied from ToStringTrait.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3617944

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

prudloff created an issue. See original summary.

bt dev2 made their first commit to this issue’s fork.

bt dev2’s picture

bt dev2 commented
Status: Active » Needs review

I removed the unnecessary cast from DateTimePlus's __toString() method.

I noticed the render() method itself doesn't have a : string return type declared. Do we need to add a : string there?

smustgrave opened merge request !16827

smustgrave’s picture

smustgrave commented
Title: Unnecessary type cast in DateTimePlus » Unnecessary type cast in added from Deprecate ToStringTrait
Status: Needs review » Needs work

Looking at #3548957: Deprecate ToStringTrait

core/lib/Drupal/Core/StringTranslation/TranslatableMarkup.php also appears to have a redundant typecast that could be removed

bt dev2’s picture

bt dev2 commented
Status: Needs work » Needs review

Removed redundant typecast from TranslatableMarkup::__toString()

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

phpstorm definitely flagged it as redundant but get a test failure so maybe it was wrong? 

   Link Base (Drupal\Tests\views\Unit\Plugin\views\field\LinkBase)
     ✘ Render null entity
       ┐
       ├ TypeError: Drupal\Core\StringTranslation\TranslatableMarkup::__toString(): Return value must be of type string, null returned
       │
       │ /builds/core/lib/Drupal/Core/StringTranslation/TranslatableMarkup.php:235
       │ /builds/core/lib/Drupal/Component/Utility/Html.php:433
       │ /builds/core/modules/views/src/Plugin/views/HandlerBase.php:227
       │ /builds/core/modules/views/src/Plugin/views/field/LinkBase.php:197
       │ /builds/core/modules/views/src/Plugin/views/field/LinkBase.php:152
       │ /builds/core/modules/views/tests/src/Unit/Plugin/views/field/LinkBaseTest.php:76
prudloff’s picture

prudloff commented

This probably means the type we document in the docblock is not always respected.

zeeshan_khan made their first commit to this issue’s fork.

zeeshan_khan opened merge request !16955

smustgrave’s picture

smustgrave commented

@zeeshan_khan can you please stop going around opening MRs where ones already exist please.

zeeshan_khan’s picture

zeeshan_khan commented

@smustgrave You're right, I shouldn't have opened a second MR - sorry about that. Closing mine. One thing worth flagging on !16827 itself: it also touched TranslatableMarkup::__toString(), which is what broke LinkBaseTest - that part might be worth dropping from !16827 and tracking separately." Post that as a comment on !16827 or the issue, not as its own MR.

quietone’s picture

quietone commented
Issue tags: +Needs title update

The title doesn't make sense to me.

prudloff’s picture

prudloff commented
Title: Unnecessary type cast in added from Deprecate ToStringTrait » Unnecessary type cast in DateTimePlus
Status: Needs work » Needs review
Issue tags: -Needs title update

I reverted the title.