Skip to content
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.

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

[5.0] [Debug] Add parameter type-hints where possible #32217

Closed
wants to merge 3 commits into from

Conversation

Matts
Copy link

@Matts Matts commented Jun 27, 2019

Q A
Branch? 5.0
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #32179
License MIT
Doc PR symfony/symfony-docs#...

Added scalar typehints to Debug (DebugBundle required no work)

@Matts Matts changed the base branch from 4.4 to master June 27, 2019 11:12
@Matts Matts force-pushed the improvement/debug-type-hints branch 2 times, most recently from 792401e to 6b0e323 Compare June 27, 2019 11:20
@Matts
Copy link
Author

Matts commented Jun 27, 2019

I did not change the BufferingLogger as this extends AbstractLogger -> LoggerInterface, and this is part of the Psr spec. Should this be changed as well? I think this could have big impact?

@Matts Matts changed the title [Debug] Add scalar typehints where possible [5.0] [Debug] Add scalar typehints where possible Jun 27, 2019
@derrabus derrabus mentioned this pull request Jun 27, 2019
57 tasks
@Matts Matts changed the title [5.0] [Debug] Add scalar typehints where possible [5.0] [Debug] Add parameter type-hints where possible Jun 28, 2019
@@ -112,7 +112,7 @@ class ErrorHandler
*
* @return self The registered error handler
*/
public static function register(self $handler = null, $replace = true): self
public static function register(self $handler = null, bool $replace = true): self
Copy link
Member

Choose a reason for hiding this comment

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

now that this class has been deprecated, all changes should be reverted (and the class should be removed actually)
see other classes too (should be easy to spot by rebasing)

Copy link
Author

Choose a reason for hiding this comment

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

Reverted all changes in class. With deprecated you mean final? I couldn't find any others in the namespace.

Copy link
Member

Choose a reason for hiding this comment

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

no, with deprecated, we mean deprecated.

@nicolas-grekas nicolas-grekas added this to the 5.0 milestone Jun 28, 2019
@Matts Matts force-pushed the improvement/debug-type-hints branch from d1aea83 to c5756ad Compare July 17, 2019 13:15
@@ -129,21 +129,19 @@ public static function disable()
}

/**
* @return string|null
* @param string $class
Copy link
Member

Choose a reason for hiding this comment

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

this is wrong. You must not remove the the doc for the return type (and adding the doc for the param is useless now, as it duplicates the typehint)

@@ -60,11 +60,11 @@ public function __construct(bool $debug = true, string $charset = null, $fileLin
*
* @param bool $debug Enable/disable debug mode, where the stack trace is displayed
* @param string|null $charset The charset used by exception messages
* @param string|null $fileLinkFormat The IDE link template
* @param string|FileLinkFormatter|null $fileLinkFormat The IDE link template
Copy link
Member

Choose a reason for hiding this comment

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

this should be added in older branches

@nicolas-grekas
Copy link
Member

This PR is going to be obsoleted by #32471

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Jul 18, 2019

The Debug component is now gone, long live ErrorHandler. New PR welcome I suppose on branch 4.4 (please have a look at comments here of course)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants