added return type declaration sniff #1219

akvankorlaar · 2016-11-23T10:59:55Z

Created according to: https://github.com/php-fig/fig-standards/blob/master/proposed/extended-coding-style-guide.md

For return type declarations, this sniff checks the spacing between the functions closing parenthesis, and the colon. It also checks the spacing between the colon and the return type. The spacings are still configurable

It does not check if the colon and return type are on the same line as the closing parenthesis. I can also add that if required.. but maybe that has to be placed in a separate sniff?

Edit: Removed requirement that return type declaration should be lowercase, because object names can also be in the return type declaration

Update: Added fixer for return type declaration sniff

aik099 · 2016-11-23T11:20:42Z

CodeSniffer/Standards/Generic/Sniffs/Functions/ReturnTypeDeclarationSniff.php

+                 T_WHITESPACE,
+                );
+
+        $a   = $this->closingParenthesisColonSpacing;


Better not to use short variable names (less than 3 symbols) to ease code reading for others.

Also the a doesn't really tell what's stored in it.

aik099 · 2016-11-23T11:21:18Z

CodeSniffer/Standards/Generic/Sniffs/Functions/ReturnTypeDeclarationSniff.php

+            } else if ($tokens[$nextSeparator]['code'] === T_RETURN_TYPE) {
+                $b   = $acc;
+                $acc = '';
+                if (ctype_lower($tokens[$nextSeparator]['content'] === false)) {


Will ctype_lower function exist for every PHP version/build?

Hey thanks for the reply.

ctype_lower is PHP 4 >= 4.0.4, PHP 5, PHP 7. Is that acceptable?

What I mean if that function always present or in some PHP builds (e.g. on CentOS) it might not be available by default.

its not available on FreeBSD, so I changed it to preg_match just in case
http://php.net/manual/en/ctype.installation.php

Also in if (ctype_lower($tokens[$nextSeparator]['content'] === false)) { line you seem to have misplaced the parenthesis and $tokens[$nextSeparator]['content'] === false goes as argument to ctype_lower.

What is the thing you're checking with it, that it's a lowercased text? I guess you can safely do strtolower($tokens[$nextSeparator]['content']) === $tokens[$nextSeparator]['content'] instead, because:

it's faster then preg_match

we can safely use non-utf8 friendly function because we know all types use english letters only

alright, changed it to strtolower

Good. But if none of tests were failing, then probably that lowercasing code is never used.

I already added tests for it when I changed it to preg_match

aik099 · 2016-11-23T11:22:57Z

CodeSniffer/Standards/PHPCS/ruleset.xml

@@ -33,6 +33,7 @@
 <rule ref="Generic.NamingConventions.ConstructorName"/>
 <rule ref="Generic.PHP.DeprecatedFunctions"/>
 <rule ref="Generic.PHP.LowerCaseKeyword"/>
+ <rule ref="Generic.Functions.ReturnTypeDeclaration"/>


You shouldn't change any of existing rulesets.

Alright. I guess thats also the reason the builds fail

Hostnet ported squizlabs/PHP_CodeSniffer#1219

Nicolai- · 2017-01-05T11:57:42Z

Looking forward to this one getting released 👍

TomasVotruba · 2017-04-25T21:01:45Z

For inspiration and already to use: https://github.com/slevomat/coding-standard/blob/master/SlevomatCodingStandard/Sniffs/TypeHints/ReturnTypeHintSpacingSniff.php

hboomsma · 2017-04-26T06:09:27Z

The implementation from the pull request is also ready for use at: https://github.com/hostnet/phpcs-tool/blob/master/src/Hostnet/Sniffs/Functions/ReturnTypeDeclarationSniff.php

jrfnl · 2017-08-04T11:57:30Z

CodeSniffer/Standards/Generic/Docs/Functions/ReturnTypeDeclarationStandard.xml

+        Return type declarations are optional, and have a specified amount of spaces between the function's closing parenthesis and colon, and a
+        specified amount of spaces between the colon and the return type.
+        The default is zero spaces between closing parenthesis and colon and one space between colon and return type.
+        The return type should be lowercase.


You may want to update this line in the description to reflect that class names don't have to be lowercase.

GaryJones · 2017-08-04T12:05:15Z

CodeSniffer/Standards/Generic/Sniffs/Functions/ReturnTypeDeclarationSniff.php

+     *
+     * @return int position within the token stack
+     */
+    private function _getCharachterAfterReturnTypeDeclaration(PHP_COdeSniffer_File $phpcsFile, array $tokens, $stackPtr)


Incorrect spelling of "Character" in method name, method usage and docs.

josephzidell · 2018-03-12T18:01:21Z

Are there plans to merge this?

gsherwood · 2018-03-12T22:25:07Z

This one slipped past me after the latest changes. Thanks for bumping it again. I'll add it to the 3.3.0 roadmap.

jrfnl · 2018-03-13T04:16:09Z

Before this sniff can be merged, it will need to be updated for the changes in PHPCS 3.x. The code style (short vs long arrays + array indentation) will also need to be fixed.

Aside from that, I personally would like to see some unit tests with comments in the function declaration / trailing comments / PHPCS 3.2.x style annotations as those may well cause fixer conflicts if these are not being taken into account by the sniff.

Edit: Removed requirement that return type declaration should be lowercase, because object names can also be in the return type declaration

These is a separate PR open which addresses this - #1685 - and takes mixed case object names into account.

gsherwood · 2018-03-18T23:55:45Z

I've done a bit of a review of this sniff and I don't think it is ready to merge. Apart from @jrfnl 's comment about needing some more unit tests to check what happens with auto-fixing when comments and PHPCS annotations are thrown in, I also have some other comments:

I don't know why the unit tests are across different files. I think they should be merged for easy of maintenance, unless I've missed something.
The public properties that allow a ruleset to override the spacing should be ints and not strings. People are either going to want n number of spaces or a newline, but not a specific string.
There should be an ignoreNewlines public property like some of the others sniffs have. This solves the problem where you weren't sure if you should report an error if they are not on the same line. A good example of how sniffs use this property, and generate error messages that include "n spaces or "newline" is: https://github.com/squizlabs/PHP_CodeSniffer/blob/master/src/Standards/Generic/Sniffs/WhiteSpace/ArbitraryParenthesesSpacingSniff.php and is documented here: https://github.com/squizlabs/PHP_CodeSniffer/wiki/Customisable-Sniff-Properties#genericwhitespacearbitraryparenthesesspacing
Just some coding standard fixes needed as well as things have changed since this was first written.

I'm going to remove this from the 3.3.0 milestone as I'm not sure how long this will take to discuss and change. I'll re-add if everything gets sorted out before the 3.3.0 release.

akvankorlaar · 2018-03-23T13:51:18Z

So Ive updated the commit. Updated some things that changed since Ive last committed, put all the tests in 1 file and did some rewriting. The configurable space properties are now ints, and added an IgnoreNewLines property.

Aside from that, I personally would like to see some unit tests with comments in the function declaration / trailing comments / PHPCS 3.2.x style annotations as those may well cause fixer conflicts if these are not being taken into account by the sniff.

Not sure what you mean. Can you give me examples?

gsherwood · 2018-05-21T05:16:12Z

Can you give me examples?

Sometimes the auto-fixers can get confused when you randomly put comments (//, /* */ and /** */ styles) throughout the function declaration. You can also place comments like // phpcs:ignore on lines to ignore all coding standards on that line.

After fixing, it's important that comments are not accidentally removed, or inline comments moved and are now commenting out code, or a phpcs:ignore annotation moved to a different line where it now means something else.

Those are the kind of unit test example @jrfnl was talking about. I haven't had a chance to try this sniff out yet and see if those test cases are working.

akvankorlaar · 2021-11-24T10:46:50Z

Closed this PR because I completely forgot that this was still open, and Ive opened this PR 5 years ago (!). Its been a few years since Ive coded much in PHP, so Im not really familiar with what the best practices are now. If someone else wants to pick this up, feel free to do so.

aik099 reviewed Nov 23, 2016

View reviewed changes

hboomsma mentioned this pull request Nov 29, 2016

Hostnet ported squizlabs/PHP_CodeSniffer#1219 hostnet/phpcs-tool#3

Merged

akvankorlaar added a commit to hostnet/phpcs-tool that referenced this pull request Nov 29, 2016

Merge pull request #3 from hboomsma/return

ec8c908

Hostnet ported squizlabs/PHP_CodeSniffer#1219

jrfnl reviewed Aug 4, 2017

View reviewed changes

jrfnl mentioned this pull request Aug 4, 2017

Add ReturnType Sniff WordPress/WordPress-Coding-Standards#1084

Closed

GaryJones suggested changes Aug 4, 2017

View reviewed changes

gsherwood added this to the 3.3.0 milestone Mar 12, 2018

gsherwood removed this from the 3.3.0 milestone Mar 18, 2018

gsherwood added Type: enhancement Status: awaiting feedback labels Mar 18, 2018

added return type declaration sniff

343e982

gsherwood removed the Status: awaiting feedback label Apr 17, 2018

tomasfejfar mentioned this pull request Aug 3, 2018

init keboola/wr-slack#1

Merged

duncan3dc mentioned this pull request Aug 12, 2018

[PSR12] Add return type sniff #2118

Closed

akvankorlaar closed this Nov 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added return type declaration sniff #1219

added return type declaration sniff #1219

akvankorlaar commented Nov 23, 2016 •

edited

Loading

aik099 Nov 23, 2016

aik099 Nov 23, 2016

akvankorlaar Nov 23, 2016

aik099 Nov 23, 2016

akvankorlaar Nov 24, 2016

aik099 Nov 24, 2016

akvankorlaar Nov 24, 2016

aik099 Nov 24, 2016

akvankorlaar Nov 24, 2016

aik099 Nov 23, 2016

akvankorlaar Nov 23, 2016

Nicolai- commented Jan 5, 2017

TomasVotruba commented Apr 25, 2017

hboomsma commented Apr 26, 2017

jrfnl Aug 4, 2017

GaryJones Aug 4, 2017

josephzidell commented Mar 12, 2018

gsherwood commented Mar 12, 2018

jrfnl commented Mar 13, 2018

gsherwood commented Mar 18, 2018

akvankorlaar commented Mar 23, 2018 •

edited

Loading

gsherwood commented May 21, 2018

akvankorlaar commented Nov 24, 2021 •

edited

Loading

added return type declaration sniff #1219

added return type declaration sniff #1219

Conversation

akvankorlaar commented Nov 23, 2016 • 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nicolai- commented Jan 5, 2017

TomasVotruba commented Apr 25, 2017

hboomsma commented Apr 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josephzidell commented Mar 12, 2018

gsherwood commented Mar 12, 2018

jrfnl commented Mar 13, 2018

gsherwood commented Mar 18, 2018

akvankorlaar commented Mar 23, 2018 • edited Loading

gsherwood commented May 21, 2018

akvankorlaar commented Nov 24, 2021 • edited Loading

akvankorlaar commented Nov 23, 2016 •

edited

Loading

akvankorlaar commented Mar 23, 2018 •

edited

Loading

akvankorlaar commented Nov 24, 2021 •

edited

Loading