- Issue created by @ptmkenny
- @ptmkenny opened merge request.
- Status changed to Needs review
over 1 year ago 1:53am 2 February 2023 - Status changed to Postponed: needs info
over 1 year ago 8:11pm 9 February 2023 - πΊπΈUnited States smustgrave
So I searched the repo and phpstan-baseline and am not seeing this error? If it were active believe it would of been noticed during test runs.
Where are you seeing this?
- Status changed to Needs review
over 1 year ago 8:52am 10 February 2023 - π―π΅Japan ptmkenny
@smustgrave I'm sorry for not being more clear. This happens when strict types is enabled (
declare(strict_types=1);
), and I've updated the IS.A quick grep of core for
setErrorByName()
shows numerous locations where a translated string is passed, so I think it should be documented that this is an OK thing to do. - Status changed to RTBC
over 1 year ago 5:39pm 11 February 2023 - πΊπΈUnited States smustgrave
Thank you for updating the issue summary.
- Status changed to Needs work
over 1 year ago 3:49pm 13 February 2023 - π¬π§United Kingdom catch
Left a comment on the MR - should use MarkupInterface or Stringable rather than the actual class here.
- First commit to issue fork.
- last update
11 months ago 29,653 pass - Open on Drupal.org βEnvironment: PHP 8.2 & MySQL 8last update
11 months ago Not currently mergeable. 5:06 41:28 Running- Status changed to Needs review
9 months ago 11:05am 3 December 2023 - π―π΅Japan ptmkenny
Setting back to "Needs review" since fgm changed it to Stringable. Note, however, that the linked related issue https://www.drupal.org/project/drupal/issues/3175012 π Update PHPDoc for DataDefinition Fixed used the full class name, which is why I originally chose to do it that way.
- Status changed to RTBC
9 months ago 8:39pm 3 December 2023 - last update
9 months ago 29,722 pass - Status changed to Needs work
9 months ago 11:58pm 4 December 2023 - πΊπΈUnited States xjm
This seems like an issue that is possibly all over the codebase, with changes being made to only one file, which is never how we scope these sorts of cleanups.
Can someone try to determine a list of places where PHPDoc has this mistake for stuff that accepts markup objects? Then we can make a core-wide decision on whether to use
\Stringable
or\MarkupInterface
.If I'm wrong and this is literally the only class with the issue it can be set back to RTBC.
- π―π΅Japan ptmkenny
Thank you for the feedback.
How might one identify this mistake on the codebase level?
I just tried:
grep "@param string \$message" * -r grep "@param string \$label" * -r
And there are a ton of results.
Obviously I haven't read them all, but for the case of $message, a lot of them are either tests or exceptions, like these:
core/lib/Drupal/Core/Form/EnforcedResponseException.php: * @param string $message core/lib/Drupal/Core/Form/FormAjaxException.php: * @param string $message core/lib/Drupal/Core/Form/FormStateInterface.php: * @param string $message core/lib/Drupal/Core/Form/FormStateInterface.php: * @param string $message core/lib/Drupal/Core/Form/Exception/BrokenPostRequestException.php: * @param string $message core/lib/Drupal/Core/Logger/LogMessageParserInterface.php: * @param string $message core/lib/Drupal/Core/Test/AssertMailTrait.php: * @param string $message core/lib/Drupal/Core/Test/AssertMailTrait.php: * @param string $message core/lib/Drupal/Core/Test/AssertMailTrait.php: * @param string $message core/lib/Drupal/Core/Config/ConfigImporter.php: * @param string $message core/lib/Drupal/Core/Security/StaticTrustedCallbackHelper.php: * @param string $message core/lib/Drupal/Core/Security/RequestSanitizer.php: * @param string $message core/lib/Drupal/Core/TypedData/Validation/ConstraintViolationBuilder.php: * @param string $message core/lib/Drupal/Core/Mail/Plugin/Mail/PhpMail.php: * @param string $message core/lib/Drupal/Core/ParamConverter/ParamNotConvertedException.php: * @param string $message core/lib/Drupal/Core/Theme/MissingThemeDependencyException.php: * @param string $message core/lib/Drupal/Core/FileTransfer/FileTransferException.php: * @param string $message core/lib/Drupal/Core/Queue/SuspendQueueException.php: * @param string $message core/lib/Drupal/Core/Queue/DelayedRequeueException.php: * @param string $message core/lib/Drupal/Core/Installer/Exception/InstallerException.php: * @param string $message core/lib/Drupal/Core/Ajax/MessageCommand.php: * @param string $message core/lib/Drupal/Core/Utility/Error.php: * @param string $message
- πΊπΈUnited States xjm
The key sentence from the IS is:
For example, code in a custom form validator will give an error when strict types is enabled:
So based on that, enable strict types, use PHPStan maybe, and filter for the appropriate error for strings. Ignore any issues in tests as those are being worked on already. Get a list of APIs where the docs are wrong, and then we can discuss at a high level what our approach should be and how we should scope the work. Thanks!
- πΊπΈUnited States xjm
Or, if PHPStan doesn't help us out here, we could also gather data in two other ways:
- Turn on strict typing and run the test suite once π± [meta] Fix strict type errors in kernel tests Active is complete.
- Investigate key UI-related elements that we know receive Markup objects.
But my bet is PHPStan will be able to get this data for us.
- Status changed to Active
9 months ago 3:58am 5 December 2023 - π―π΅Japan ptmkenny
ptmkenny β changed the visibility of the branch setErrorByName to hidden.
- π―π΅Japan ptmkenny
Well, despite #12 stating that changes to one file are never made for this type of cleanup, π FormStateInterface::setError*() PHPDoc are incorrect Fixed was committed for Drupal 10.3.2/11.0.1, fixing the original issue I reported here. So I'm closing the MR and I guess we leave this open as a meta issue?