- Issue created by @fgm
- Assigned to Anchal_gupta
- Issue was unassigned.
- Status changed to Needs review
about 1 year ago 11:21am 7 July 2023 - last update
about 1 year ago 29,439 pass - 🇫🇷France fgm Paris, France
What about the return type ? I thought that starting with D10 we were adding return type hints, are we not ? (not saying we do: I just wonder)
Otherwise LGTM.
- Status changed to Needs work
about 1 year ago 3:25pm 7 July 2023 - Status changed to Needs review
about 1 year ago 8:08am 10 July 2023 - last update
about 1 year ago Custom Commands Failed - last update
about 1 year ago Custom Commands Failed - Status changed to Needs work
about 1 year ago 8:21am 10 July 2023 - 🇫🇷France fgm Paris, France
@AkashKumar07 as you can see this patch doesn't pass tests. Can you fix it before we can review ?
Once you get an error, like this, iou may want to test locally on your machine before submitting the patch to drupalci, as you then see where the failure is, so you don't have to run the whole test suite, but just one test that failed on CI, and it's much faster to run locally.
- last update
about 1 year ago Custom Commands Failed - 🇫🇷France fgm Paris, France
Look at the results of that patch: there are more errors: https://www.drupal.org/pift-ci-job/2712812 →
You could run phpstan locally yourself to get results faster: just install phpstan-drupal, e.g. using the Extension Installer https://github.com/phpstan/extension-installer : that way you can get the same results as those given by DrupalCI on your own machine.
- Status changed to Needs review
about 1 year ago 11:55am 13 July 2023 - last update
about 1 year ago 29,444 pass - 🇫🇷France fgm Paris, France
Actually, there are a couple strings returned too, so the patch is a bit larger.
- Status changed to RTBC
about 1 year ago 1:44pm 13 July 2023 - 🇺🇸United States smustgrave
Thanks @fgm for taking care of this
Tests are all green so adding typehint is good.
Short and sweet!
- Status changed to Needs work
about 1 year ago 10:46am 14 July 2023 - 🇬🇧United Kingdom catch
We can't add the return type hint to the interface in a minor release. See 📌 Enable existing interfaces to add return type hints with a deprecation message for implementors Active .
Also wonder, could we use Stringable instead of TranslatableMarkup here? It'd be more generic.
- 🇫🇷France fgm Paris, France
Thanks. I see that 📌 Enable existing interfaces to add return type hints with a deprecation message for implementors Active does not currently suggest a path forward. So what should we do here ?
- Just fix the phpdoc ? :-(
- Define and add the
[#ReturnTypeHintDeprecation]
? That seems out of scope.
Regarding
\Stringable
, the current phpdoc incorrectly specifiesstring|null
, so it does not even allow it, convenient though it would be. If we dig a little, thestring
return is only ever used inFilterHtml
, with all other core implementations using either naked returns (soNULL
) orTranslatableMarkup
so we could just as well consider it is an error and only allow?\TranslatableMarkup
.But since this is what we document the interface to be, we must probably assume that contrib/custom devs have looked at the core implementations and just handled the actual returns, so they also return
?\TranslatableMarkup
. Should be move to\Stringable<\code>, that would be the type to add to the typed return in 11.x, and it could in the future break any contrib actually using any of the public methods on <code>?\TranslatableMarkup
(because that's what core actually used) whenever someone decides to update core to return a\Stringable
that is not also a\TranslatableMarkup
because, hey, that's what core documents... so I think the safest PHPdoc signature would be?\Drupal\Core\StringTranslation\TranslatableMarkup
.I removed the novice tag because that's no longer a novice question.