Replace more docblocks by type-hints #24722

nicolas-grekas · 2017-10-28T18:18:20Z

Q	A
Branch?	master
Bug fix?	no
New feature?	no
BC breaks?	yes (but only on constructors and final methods)
Deprecations?	no
Tests pass?	yes
Fixed tickets	-
License	MIT
Doc PR	-

nicolas-grekas · 2017-10-28T18:58:36Z

Status: needs work

To be rebase on top of #24723 #24724 #24725 #24726, and failure investigated.

kaznovac · 2017-10-31T12:53:09Z

good initiative, but the parameter documentation should be preserved IMHO

nicolas-grekas · 2017-11-05T20:25:41Z

src/Symfony/Component/HttpFoundation/Response.php

    {
        try {
            return $this->headers->getDate('Expires');
        } catch (\RuntimeException $e) {
            // according to RFC 2616 invalid date formats (e.g. "0" and "-1") must be treated as in the past
-            return \DateTime::createFromFormat(DATE_RFC2822, 'Sat, 01 Jan 00 00:00:00 +0000');
+            return \DateTime::createFromFormat('U', time() - 172800);


This change is required to prevent an integer overflow on 32bit systems.

nicolas-grekas · 2017-11-06T19:24:50Z

Status: needs review (enjoy ;-))

Docblocks are really different than actual type hints, their respective accuracies have nothing in common.

stof · 2017-11-07T11:46:44Z

We should of course use them from the start for any new API added in 4.1+

This PR was merged into the 4.0-dev branch. Discussion ---------- Replace more docblocks by type-hints | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | yes (but only on constructors and final methods) | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - Commits ------- aaf2265 Replace more docblocks by type-hints

tomasfejfar · 2017-11-07T21:57:05Z

src/Symfony/Bridge/Doctrine/DependencyInjection/CompilerPass/DoctrineValidationPass.php

     */
-    private function updateValidatorMappingFiles(ContainerBuilder $container, $mapping, $extension)
+    private function updateValidatorMappingFiles(ContainerBuilder $container, string $mapping, string $extension)


shouldn't this have void return typehint?

I find void useless, so I didn't add any.
If you want to make it your battle, please do :)

Method without void can mean two types of things - either it's return type void, or it's just not filled in or the actual return type is in DocBlock. That's a lot of "or"s :)

For me, it would make sense to add it if it were to be enforced by phpcs (method either has return type hint, or @return docblock). Is there a plan to enforce return typehints?

no plan I know about

Tobion · 2017-11-08T04:10:21Z

src/Symfony/Component/Form/Extension/Core/DataTransformer/BaseDateTimeTransformer.php

@@ -36,7 +36,7 @@
     * @throws UnexpectedTypeException  if a timezone is not a string
     * @throws InvalidArgumentException if a timezone is not valid
     */
-    public function __construct($inputTimezone = null, $outputTimezone = null)
+    public function __construct(string $inputTimezone = null, string $outputTimezone = null)
    {
        if (null !== $inputTimezone && !is_string($inputTimezone)) {
            throw new UnexpectedTypeException($inputTimezone, 'string');


this check is useless now as it cannot happen anymore

Tobion · 2017-11-08T04:12:17Z

...onent/Form/Tests/Extension/Core/DataTransformer/DateTimeToLocalizedStringTransformerTest.php

@@ -307,15 +307,15 @@ public function testReverseTransformWrapsIntlErrors()
    }

    /**
-     * @expectedException \Symfony\Component\Form\Exception\UnexpectedTypeException
+     * @expectedException \TypeError
     */
    public function testValidateDateFormatOption()


those tests are useless now as they only test php internal behavior

I'm sure there are more left over, PR is so big...
Additional PR welcomed :)

@Tobion

This PR was merged into the 4.0-dev branch. Discussion ---------- Remove some unneeded checks/tests | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - As spotted by @Tobion in #24722 (review) Commits ------- 05681ec Remove some unneeded checks/tests

TomasVotruba · 2017-11-10T18:13:38Z

Hey @nicolas-grekas , I'm curious, do you use (at least help of) automated tools for this, like TypeHintDeclarationSniff?

Checks for useless doc comments. If the native method declaration contains everything and the phpDoc does not add anything useful, it's reported as useless and can optionally be automatically removed with phpcbf.

I plan to add fixer who would take care of this deprecated-packages/symplify#416, because it's too time demanding and can be mostly automated, so I look for inspiration.

nicolas-grekas · 2017-11-10T18:31:17Z

None, had to stay concentrated a few hours on it, would love a tool to help keep the docblocks low!

TomasVotruba · 2017-11-10T20:01:38Z

Holly cow 😲 ! I admire your persistence

I did something similar few days ago on php-ml: https://github.com/php-ai/php-ml/pull/145 with following setup:

composer require symplify/easy-coding-standard --dev

# clean-docs.neon
checkers:
    SlevomatCodingStandard\Sniffs\TypeHints\TypeHintDeclarationSniff:
        enableEachParameterAndReturnInspection: true

vendor/bin/ecs check src --config clean-docs.neon
# and to fix
vendor/bin/ecs check src --config clean-docs.neon --fix

TypeHintDeclarationSniff misses many cases and removes some annotations so it required ~40 minutes extra, but I'm working on fixer that should decrease this to 2 minutes extra (just scrolling the diff ;)).

Feel free to try this and let me know how it works for you.

TomasVotruba · 2017-11-11T21:36:45Z

@nicolas-grekas Finished in dev version: https://github.com/Symplify/CodingStandard#block-comment-should-only-contain-useful-information-about-types

Used in these PR:

…udaltsov) This PR was merged into the 4.1 branch. Discussion ---------- [Process] Allow to pass non-string arguments to Process | Q | A | ------------- | --- | Branch? | 4.1 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #28609 | License | MIT | Doc PR | Sometimes you might pass integers, floats, nulls as command arguments to the Process constructor. Before 4.0 and #24722 that worked fine. Now it throws because of an invalid type. I think we can drop the type hint here safely and stringify the value implicitly in the method. That will be a little more convenient. Commits ------- acf8b83 Remove Process::escapeArgument argument type hint

@xabbuh

This PR was merged into the 4.4 branch. Discussion ---------- Fine tune constructor types | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | no | New feature? | no  | BC breaks? | no  | Deprecations? | no  | Tests pass? | yes  | Fixed tickets | | License | MIT | Doc PR | symfony/symfony-docs#...  Fine tunes some constructor types that have been added in #24722 Form names as integer was only a workaround as forms names are used as array keys which get transformed to int. So it was added as a workaround in #6355 (comment) With typehints added in #24722 those were mostly auto-cast anyway, e.g. in FormBuilder. There are only a few integer form names remaining documented, in the main entry points of the Form component (`\Symfony\Component\Form\FormInterface::add`). Internally it's always a string now. So I could remove some int docs which also fixes #30032 what @xabbuh tried to do. Some of these changes we're just not done before because of broken tests. It's mainly a missing explicit mock for `TranslationInterface::trans` which returned null. If we had return types hints in interfaces, this wouldn't happen. Commits ------- 507794a Fine tune constructor types

…e Segatori, tigitz) This PR was squashed before being merged into the 5.0-dev branch (closes #32273). Discussion ---------- [Process] [5.0] Replace docblocks by type-hints | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no  | Tests pass? | yes  | Fixed tickets | continuation of #24722 and checks for #32179 | License | MIT | Doc PR | N/A This PR adds replace docblocks by type hints in the Process component. Some docblocks without valuable information got also removed. Commits ------- 5c964c5 [Process] [5.0] Replace docblocks by type-hints

…ippe Segatori) This PR was merged into the 5.0-dev branch. Discussion ---------- [HttpKernel] [5.0] Replace docblocks by type-hints | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no  | Tests pass? | yes  | Fixed tickets | continuation of #24722 and checks for #32179 | License | MIT | Doc PR | N/A This PR replaces docblocks by type hints in the HttpKernel component considering #32179. Some docblocks without valuable information got also removed.  Commits ------- 9e570a2 [Http-Kernel][5.0] Add type-hints

…, Tobion) This PR was squashed before being merged into the 5.0-dev branch (closes #32525). Discussion ---------- [Intl][5.0] Add parameters type-hints | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no  | Tests pass? | yes  | Fixed tickets | continuation of #24722 and checks for #32179 | License | MIT | Doc PR | N/A This PR replaces docblocks by type hints in the Intl component considering #32179. Some docblocks without valuable information got also removed. Commits ------- ce79f4b [Intl][5.0] Add parameters type-hints

carsonbot added Status: Needs Review BC Break labels Oct 28, 2017

nicolas-grekas added this to the 4.0 milestone Oct 28, 2017

nicolas-grekas force-pushed the final-types branch 4 times, most recently from ec3b7c0 to e32d019 Compare October 28, 2017 18:39

nicolas-grekas mentioned this pull request Oct 28, 2017

[FrameworkBundle] Make Controller helpers final #24407

Merged

nicolas-grekas force-pushed the final-types branch from e32d019 to eab1f27 Compare October 28, 2017 18:44

carsonbot added Status: Needs Work and removed Status: Needs Review labels Oct 28, 2017

nicolas-grekas force-pushed the final-types branch 3 times, most recently from 768004a to 55854e7 Compare November 5, 2017 20:24

nicolas-grekas commented Nov 5, 2017

View reviewed changes

nicolas-grekas force-pushed the final-types branch 2 times, most recently from c27f96a to 342ae44 Compare November 6, 2017 15:02

nicolas-grekas added Status: Needs Review and removed Status: Needs Work labels Nov 6, 2017

fabpot approved these changes Nov 6, 2017

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Nov 6, 2017

nicolas-grekas force-pushed the final-types branch from 342ae44 to 0f3edad Compare November 6, 2017 20:10

stof approved these changes Nov 7, 2017

View reviewed changes

nicolas-grekas force-pushed the final-types branch from 0f3edad to daf1157 Compare November 7, 2017 13:36

Replace more docblocks by type-hints

aaf2265

nicolas-grekas force-pushed the final-types branch from daf1157 to aaf2265 Compare November 7, 2017 14:45

nicolas-grekas merged commit aaf2265 into symfony:master Nov 7, 2017

nicolas-grekas deleted the final-types branch November 7, 2017 14:55

tomasfejfar reviewed Nov 7, 2017

View reviewed changes

Tobion reviewed Nov 8, 2017

View reviewed changes

nicolas-grekas mentioned this pull request Nov 9, 2017

Remove some unneeded checks/tests #24876

Merged

tomasfejfar mentioned this pull request Nov 9, 2017

Enforce return type hints for Symfony 4+ #24882

Closed

ro0NL mentioned this pull request Nov 11, 2017

[Filesystem] Replace remaing docblocks by type-hints #24926

Closed

TomasVotruba mentioned this pull request Nov 12, 2017

[POC] Add PHP 7.1 typehints + remove unused doc for @param and @return, #24930

Closed

fabpot mentioned this pull request Nov 12, 2017

Release v4.0.0-BETA4 #24933

Merged

vudaltsov mentioned this pull request Oct 14, 2018

[Process] Allow to pass null arguments to Process #28863

Merged

Tobion mentioned this pull request Jun 16, 2019

Fine tune constructor types #32066

Merged

This was referenced Jun 28, 2019

[Process] [5.0] Replace docblocks by type-hints #32273

Merged

[HttpKernel] [5.0] Replace docblocks by type-hints #32276

Closed

tigitz mentioned this pull request Jul 12, 2019

[Intl][5.0] Add parameters type-hints #32525

Merged

derrabus mentioned this pull request Mar 8, 2021

Fix ConstraintViolation#getPropertyPath() to always return string #40415

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace more docblocks by type-hints #24722

Replace more docblocks by type-hints #24722

nicolas-grekas commented Oct 28, 2017 •

edited

Loading

nicolas-grekas commented Oct 28, 2017

kaznovac commented Oct 31, 2017

nicolas-grekas Nov 5, 2017

nicolas-grekas commented Nov 6, 2017 •

edited

Loading

stof commented Nov 7, 2017

tomasfejfar Nov 7, 2017

nicolas-grekas Nov 8, 2017

tomasfejfar Nov 9, 2017

nicolas-grekas Nov 9, 2017

tomasfejfar Nov 9, 2017

Tobion Nov 8, 2017 •

edited

Loading

nicolas-grekas Nov 9, 2017

Tobion Nov 8, 2017

nicolas-grekas Nov 8, 2017

TomasVotruba commented Nov 10, 2017

nicolas-grekas commented Nov 10, 2017

TomasVotruba commented Nov 10, 2017 •

edited

Loading

TomasVotruba commented Nov 11, 2017

Replace more docblocks by type-hints #24722

Replace more docblocks by type-hints #24722

Conversation

nicolas-grekas commented Oct 28, 2017 • edited Loading

nicolas-grekas commented Oct 28, 2017

kaznovac commented Oct 31, 2017

Choose a reason for hiding this comment

nicolas-grekas commented Nov 6, 2017 • edited Loading

stof commented Nov 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tobion Nov 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomasVotruba commented Nov 10, 2017

nicolas-grekas commented Nov 10, 2017

TomasVotruba commented Nov 10, 2017 • edited Loading

TomasVotruba commented Nov 11, 2017

nicolas-grekas commented Oct 28, 2017 •

edited

Loading

nicolas-grekas commented Nov 6, 2017 •

edited

Loading

Tobion Nov 8, 2017 •

edited

Loading

TomasVotruba commented Nov 10, 2017 •

edited

Loading