8000 add parameter type declarations to private methods by xabbuh · Pull Request #32786 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

add parameter type declarations to private methods #32786

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Aug 1, 2019
Merged

add parameter type declarations to private methods #32786

merged 1 commit into from
Aug 1, 2019

Conversation

xabbuh
Copy link
Member
@xabbuh xabbuh commented Jul 27, 2019
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

@xabbuh xabbuh added this to the next milestone Jul 27, 2019
@xabbuh xabbuh mentioned this pull request Jul 27, 2019
Merged
@xabbuh xabbuh force-pushed the private-methods-parameter-type-declarations branch 3 times, most recently from 6c01625 to 6462261 Compare July 27, 2019 21:48
derrabus
derrabus requested changes Jul 28, 2019
View reviewed changes
@xabbuh xabbuh force-pushed the private-methods-parameter-type-declarations branch 10 times, most recently from c35772b to 2d7d64b Compare July 30, 2019 15:39
@xabbuh xabbuh changed the title [WIP] add parameter type declarations to private methods add parameter type declarations to private methods Jul 30, 2019
@xabbuh xabbuh marked this pull request as ready for review July 30, 2019 15:49
@xabbuh xabbuh requested a review from dunglas as a code owner July 30, 2019 15:49
@xabbuh
Copy link
Member Author
xabbuh commented Jul 30, 2019

Status: Needs review

nicolas-grekas
nicolas-grekas requested changes Jul 30, 2019
View reviewed changes
Copy link
Member
@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

deps=low line on Travis consistently fails with a timeout, this looks related, isn't it?

@xabbuh xabbuh force-pushed the private-methods-parameter-type-declarations branch 2 times, most recently from 99487df to 980899e Compare July 31, 2019 08:56
@xabbuh xabbuh force-pushed the private-methods-parameter-type-declarations branch 4 times, most recently from 84443ec to 130c4a2 Compare July 31, 2019 18:40
@xabbuh xabbuh force-pushed the private-methods-parameter-type-declarations branch from 130c4a2 to 1b2aaa4 Compare July 31, 2019 18:51
nicolas-grekas
nicolas-grekas approved these changes Jul 31, 2019
View reviewed changes
Copy link
Member
@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(we investigated that the failure is a pure CI artefact and should be fixed after merge)

@Tobion
Copy link
Contributor
Tobion commented Aug 1, 2019 •
edited
Loading

I think it's good to do that here. But also I think it's a huge risk to add incompatibilities that are not covered by tests. I would not trust the phpdocs all the time and private functions mostly had no phpdocs at all. So the only way the add types there is by tracing all the entry points individually which I guess was a huge pain and is almost impossible to review.
For changes like this, I wish we had phpstan active. That would give us much more confidence.

@fabpot
Copy link
Member
fabpot commented Aug 1, 2019

As this is for 4.4, we will have plenty of people testing this in real life, which is way much better than any static analysis tool.

@fabpot
Copy link
Member
fabpot commented Aug 1, 2019

Thank you @xabbuh.

@fabpot fabpot merged commit 1b2aaa4 into symfony:4.4 Aug 1, 2019
fabpot added a commit that referenced this pull request Aug 1, 2019
@fabpot
minor #32786 add parameter type declarations to private methods (xabbuh)
f830226 
This PR was merged into the 4.4 branch.

Discussion
----------

add parameter type declarations to private methods

| 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        |

Commits
-------

1b2aaa4 add parameter type declarations to private methods
@xabbuh xabbuh deleted the private-methods-parameter-type-declarations branch August 1, 2019 07:55
This was referenced Aug 4, 2019
Merged
Merged
@Tobion Tobion mentioned this pull request Aug 7, 2019
Merged
nicolas-grekas added a commit that referenced this pull request Aug 8, 2019
@nicolas-grekas
minor #33018 remove some more useless phpdocs (Tobion)
fbaf4ac 
This PR was merged into the 4.4 branch.

Discussion
----------

remove some more useless phpdocs

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #...   <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

Fix some leftovers from #32974 and #32786

Commits
-------

9be4d17 remove some more useless phpdocs
ostrolucky
ostrolucky reviewed Aug 18, 2019
View reviewed changes
src/Symfony/Component/HttpFoundation/Session/Storage/Handler/PdoSessionHandler.php
@@ -775,13 +771,9 @@ private function getSelectSql(): string
/**
* Returns an insert statement supported by the database for writing session data.
*
* @param string $sessionId Session ID
* @param string $sessionData Encoded session data
* @param int $maxlifetime session.gc_maxlifetime
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

quite a lot of useful phpdoc descriptions have been removed :/

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which of those would you like to keep?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maxlifetime

@nicolas-grekas nicolas-grekas modified the milestones: next, 4.4 Oct 27, 2019
hultberg pushed a commit to hultberg/symfony that referenced this pull request Sep 17, 2021
@fabpot
minor symfony#32786 add parameter type declarations to private method…
aa516b4 
…s (xabbuh)

This PR was merged into the 4.4 branch.

Discussion
----------

add parameter type declarations to private methods

| 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        |

Commits
-------

1b2aaa4 add parameter type declarations to private methods
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ostrolucky ostrolucky ostrolucky left review comments

@derrabus derrabus derrabus requested changes

@fabpot fabpot fabpot approved these changes

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@dunglas dunglas Awaiting requested review from dunglas dunglas is a code owner

Assignees
No one assigned
Labels
Status: Reviewed
Projects
None yet
Milestone
4.4
Development

Successfully merging this pull request may close these issues.
7 participants
@xabbuh @Tobion @fabpot @nicolas-grekas @ostrolucky @derrabus @carsonbot
0