Created a new Generic.WhiteSpace.LanguageConstructSpacingSniff by gmponos · Pull Request #1337 · squizlabs/PHP_CodeSniffer

gmponos · 2017-02-04T19:01:32Z

aik099 · 2017-02-04T19:54:07Z

CodeSniffer/Standards/Generic/Sniffs/WhiteSpace/LanguageConstructSpacingSniff.php

+                T_ECHO,
+                T_PRINT,
+                T_RETURN,
+                T_INCLUDE,


Not sure about include, require, include_once and require_once because they can be written in 2 ways:

as keyword: include_once 'file.php';

as function call: include_once('file.php');

In later case it will be auto-fixed as include_once ('file.php'); which will trigger warning in another sniff checking function calls.

These TOKENS already existed in the deprecated one. I just copied and added more tokens. The sniff already used to handle such cases.

Then probably I reported that issue for deprecated sniff as well. Then don't change anything here.

gmponos · 2017-02-04T19:59:34Z

Now that I think about it I must add also tests. I forgot about it.

gmponos · 2017-02-04T22:03:43Z

CodeSniffer/Standards/Generic/Tests/WhiteSpace/LanguageConstructSpacingUnitTest.inc

+return($blah);
+
+yield $blah;
+yield   $blah;


Not sure how to make this test pass for version 5.3 and 5.

gmponos · 2017-05-09T18:37:17Z

Hello... I moved this PR to use the new version of codesniffer but still I don't know how to pass PHP 5.4

Same as here

jrfnl · 2017-05-09T18:50:18Z

I don't know how to pass PHP 5.4

For most versions it is a case of cleaning up the added code (docblock at the top) - see: https://travis-ci.org/squizlabs/PHP_CodeSniffer/jobs/230469601#L224

As for PHP 5.4, see my remark in the other issue

gmponos · 2017-07-23T08:38:49Z

Since this #1513 PR was merged the tests of the current one are passing and it can now be merged I think.

jrfnl · 2017-07-23T08:53:57Z

src/Standards/Generic/Sniffs/WhiteSpace/LanguageConstructSpacingSniff.php

+                T_REQUIRE,
+                T_REQUIRE_ONCE,
+                T_NEW,
+                T_YIELD,


What about also adding T_YIELD_FROM ?

I saw the conversation on the other PR and I also thought about it but I need to understand something here is the T_YIELD_FROM equals to just from or yield from?

yield from

OK... I will need to check it out if this sniff has the same behaviour with two word tokens.. I am curious about it.

hello @jrfnl

As I suspected this sniff is not working as expected with two word tokens (check the travis)

Do you have any suggestion about this?

My suggestion would be:

Remove these test cases but add comments on the sniff and inside test and fix this on a different PR.

Remove the T_YIELD_FROM add comments on the sniff that T_YIELD_FROM needs to be added and solve this on a different PR.

@gsherwood what do you think?

@gmponos I would go with something like that:

if ($tokens[$stackPtr]['code'] === T_YIELD_FROM) { $space = substr($tokens[$stackPtr]['content'], -5, 4); if ($space !== ' ') { // error and fix } }

I know, it is only specific for that one token, but in future if we have another 'two words' token we can prepare something more generic.

I second @webimpress' suggestion here, though I would change the second line as that would still not get the right info:

$space = stri_replace(array('yield', 'from'), '', $tokens[$stackPtr]['content']); // Or maybe even better (simpler), just do: if (strtolower($tokens[$stackPtr]['content']) !== 'yield from') { // error and replace content }

// Or maybe even better (simpler), just do: if (strtolower($tokens[$stackPtr]['content']) !== 'yield from') { // error and replace content }

Oh, yeah, it even better and much clearer! Thanks @jrfnl

jrfnl · 2017-07-23T08:57:25Z

Just wondering: should the "old" sniff be removed/excluded from PHPCS native rulesets which contain it and the "new" sniff be added to them instead ?

gmponos · 2017-07-23T09:21:26Z

I have checked all the other rulesets (hope I didn't miss anything) and it does not seem like any other ruleset besided Squiz was using it.
I could also exclude it from Squiz ruleset but I was not sure if this is considered backwards compatible.

Let's say that someone has a custom ruleset and includes all the Squiz ruleset.

If he had in his custom ruleset Squiz.WhiteSpace.LanguageConstructSpacingSnif silenced or converted to warning instead of error or maybe someother case (that does not come to my mind at the moment) he will have to adjust his custom ruleset.

gmponos · 2017-07-23T11:17:50Z

Seems it still can't be merged because of this blocking issue #1572

michalbundyra · 2017-07-24T15:44:40Z

src/Standards/Generic/Sniffs/WhiteSpace/LanguageConstructSpacingSniff.php

+ * Ensures all language constructs contain a single space between themselves and their content.
+ *
+ * @author    Greg Sherwood <gsherwood@squiz.net>
+ * @copyright 2006-2015 Squiz Pty Ltd (ABN 77 084 670 600)


I think, as it is new file year here should be 2017, not 2015.

michalbundyra · 2017-07-24T15:45:32Z

src/Standards/Generic/Sniffs/WhiteSpace/LanguageConstructSpacingSniff.php

+    {
+        $tokens = $phpcsFile->getTokens();
+
+        if ($tokens[($stackPtr + 1)]['code'] === T_SEMICOLON) {


We need check first if $tokens[($stackPtr + 1)] is set.

Please have in mind that these are copy pasted from the deprecated one.

michalbundyra · 2017-07-24T15:46:19Z

src/Standards/Generic/Sniffs/WhiteSpace/LanguageConstructSpacingSniff.php

+        if ($tokens[($stackPtr + 1)]['code'] === T_WHITESPACE) {
+            $content       = $tokens[($stackPtr + 1)]['content'];
+            $contentLength = strlen($content);
+            if ($contentLength !== 1) {


Not sure about this check. What if the character after keyword is single tab (\t) or just new line (\n)?

Please have in mind that these are copy pasted from the deprecated one.

same here.

Then I would improve it, as it is a new sniff. Seems to be wrong for me, in cases I've mentioned.

First of all there is an issue risen here. If one of these two sniffs has changes/fixes then the sync between them can be easily forgotten.

I wonder if it would be better instead of copy-pasting the code from the Squiz sniff (as I did) the new Generic sniff to extend the Squiz sniff and only override the register function with more tokens.

About the corrections that you are saying I would suggest you to create a PR for the Squiz sniff and have the current PR pending until these changes are merged. Once your PR is merged we can proceed here either with the way I suggest above or by copy-pasting again the code along with the fixes. The reason is that

I like to keep my PR focused on the main target and the goal of this PR was adding more tokens. It is just a way of how I prefer to work.

Also these fixes need to be made also on the Squiz sniff.

If for those two suggestion above @gsherwood has another opinion I don't mind to go by that.

I wonder if it would be better instead of copy-pasting the code from the Squiz sniff (as I did) the new Generic sniff to extend the Squiz sniff and only override the register function with more tokens.

I personally would do it the other around: have the Generic sniff contain the code and let the Squiz version extend and overload the register() method if so desired.

Or maybe we should just update list of keywords in Squiz... ? The same I've proposed in #1492 and it looks like it will be merged for 3.1.0 release.

Or maybe we should just update list of keywords in Squiz... ?

As I said I like to keep my PR focused and that was my main goal but if you read the related issue at the top I had to go by what @gsherwood said.

I personally would do it the other around: have the Generic sniff contain the code and let the Squiz version extend and overload the register() method if so desired.

I find it reasonable.

This is resolved in another PR and I will follow the suggestion of @jrfnl about Squiz extending the Generic one.

…ore language constructs. - Squiz.Whitespace.LanguageConstructSpacing is deprecated and will extend the Generic one.

gmponos · 2017-11-29T19:34:50Z

I believe it's fine now. Hope you can check it.

gsherwood · 2018-03-22T00:33:58Z

Thanks a lot for the PR. I've merged it in now, with a couple of small changes:

I removed the old Squiz sniff so this becomes a "move" of a sniff from the Squiz standard to the Generic standard. I updated the Squiz ruleset and put a note in the changelog about the BC break.
I added support for multi-line yield from statements because if someone ever uses one, the sniff was breaking fairly badly.
When yield from is not formatted correctly, only a single error is raised rather than trying to check after the from as well. This doesn't affect auto-fixing because a second loop fixes it, but it stop multi-line yield from statements from making a mess of the error report.

The sniff will be available from version 3.3.0.

gsherwood · 2018-03-22T00:34:53Z

...and I forgot to fix the tests to support older PHP version. Will do that now.

jrfnl · 2018-03-22T00:52:16Z

@gsherwood As you say, removing the sniff is a BC-break. More than that, it is a problematic BC break for external standards which support more PHPCS versions than just the latest & greatest.

Would you please reconsider deprecating the old sniff (and letting it extend the new sniff for now) and only removing the old sniff in v 4.0.0 ?

…lder PHP versions (ref #1337)

gsherwood · 2018-03-22T02:35:27Z

Would you please reconsider deprecating the old sniff (and letting it extend the new sniff for now) and only removing the old sniff in v 4.0.0 ?

Fair point. I've restored the Squiz sniff to its old behaviour. I don't think it should implement any of the new checks, so it does not extend the new sniff.

I've also fixed the multi-line T_YIELD_FROM issue that was causing the unit tests to fail on old PHP versions.

gmponos · 2018-03-22T07:07:48Z

Thanks for merging and sorry for any inconvenience caused.

jrfnl · 2018-03-22T10:22:09Z

@gsherwood Much appreciated!

Common::prepareForOutput() replaces spaces with a middle dot character to make whitespace visible in error messages. It was added to this sniff in commit 6ef2856 ("Better support for showing spaces and newlines"), probably as a follow-up to the multi-line `yield from` support added in squizlabs/PHP_CodeSniffer 1337 (commits ce60083 and d1a9163) as all three commits are by the same author on the same day. For most PHP keywords prepareForOutput() has no effect since they don't contain whitespace. The one exception is `yield from` (T_YIELD_FROM), which, in some cases, is tokenized as a single token containing a space. In that case, prepareForOutput() would display `yield·from`, but I would argue that showing the keyword as-is is preferable even in this case. This changes the error output for `yield from` from: ``` expected "yield·from" but found "Yield·From" ``` To: ``` expected "yield from" but found "Yield From" ``` Suggested in: squizlabs/PHP_CodeSniffer 2652 It was added to this sniff in commit 6ef2856 ("Better support for showing spaces and newlines"), as a follow-up to the multi-line yield from support added in squizlabs/PHP_CodeSniffer#1337

gmponos force-pushed the issue#1317 branch from 1ad9e40 to baa0c51 Compare February 4, 2017 19:13

aik099 reviewed Feb 4, 2017

View reviewed changes

gmponos force-pushed the issue#1317 branch 2 times, most recently from ccffbbb to 91386fe Compare February 4, 2017 21:33

Created a new Generic.WhiteSpace.LanguageConstructSpacingSniff

4f4c39c

gmponos force-pushed the issue#1317 branch from 91386fe to 4f4c39c Compare February 4, 2017 21:47

gmponos commented Feb 4, 2017

View reviewed changes

gmponos added 3 commits May 9, 2017 21:18

Merged with master

59cfcf4

Converted to the new 3.x version

e36f3eb

Fix docblock for Squiz.Whitespace.LanguageConstructSpacing

391255e

Fixed the spaces on the header file

46c86fe

gsherwood added the Type: enhancement label May 9, 2017

Merge branch 'master' into issue#1317

38734cf

jrfnl reviewed Jul 23, 2017

View reviewed changes

Added T_YIELD_FROM in LanguageConstructSpacing sniff

3a0e401

gmponos mentioned this pull request Jul 23, 2017

Tests for Generic standard are not running #1572

Closed

gmponos added 2 commits July 24, 2017 18:07

Merge branch 'master' into issue#1317

e55cc96

Fix the failing tests for Generic.Whitespace.LanguageConstructSpacing

9ff1d5c

michalbundyra suggested changes Jul 24, 2017

View reviewed changes

Fixed docblocks of LanguageConstructSpacing sniffs

ac56fb7

michalbundyra mentioned this pull request Jul 24, 2017

Squiz.WhiteSpace.LanguageConstructSpacing does not properly check for tabs and newlines #1573

Merged

gsherwood added this to the 3.2.0 milestone Jul 24, 2017

Merge branch 'master' into issue#1317

c86e930

gsherwood modified the milestones: 3.2.0, 3.3.0 Nov 28, 2017

gmponos added 2 commits November 29, 2017 20:33

Merge branch 'master' into issue#1317

00feffb

- Created the Generic.Whitespace.LanguageConstructSpacing to handle m…

15941b1

…ore language constructs. - Squiz.Whitespace.LanguageConstructSpacing is deprecated and will extend the Generic one.

gmponos added 2 commits March 4, 2018 22:26

Merged with master

5983324

Fix phpcs

8260a70

gsherwood merged commit 8260a70 into squizlabs:master Mar 22, 2018

gsherwood added a commit that referenced this pull request Mar 22, 2018

Added support for multi-line YIELD FROM statements (ref #1337)

ce60083

jrfnl mentioned this pull request Mar 22, 2018

Upstream Squiz.WhiteSpace.LanguageConstructSpacing sniff removed in PHPCS 3.3.0 WordPress/WordPress-Coding-Standards#1328

Closed

gsherwood added a commit that referenced this pull request Mar 22, 2018

Multi-line T_YIELD_FROM statemenets are now replicated properly for o…

d1a9163

…lder PHP versions (ref #1337)

gsherwood added a commit that referenced this pull request Mar 22, 2018

Restored the old Squiz sniff to previous behaviour (ref #1337)

178b1bf

gsherwood added a commit that referenced this pull request Mar 22, 2018

Restored the old Squiz sniff tests to previous behaviour (ref #1337)

b6143d5

gmponos deleted the issue#1317 branch March 22, 2018 07:07

jrfnl mentioned this pull request Dec 21, 2018

Extra: Use the new Generic.WhiteSpace.LanguageConstructSpacing sniff (PHPCS 3.3.0) WordPress/WordPress-Coding-Standards#1585

Merged

jrfnl mentioned this pull request Jan 6, 2019

PHPCS 3.4.1: Use the Generic.WhiteSpace.LanguageConstructSpacing sniff WordPress/WordPress-Coding-Standards#1616

Closed

3 tasks

jrfnl mentioned this pull request Apr 14, 2025

Squiz/LanguageConstructSpacing: remove deprecated sniff PHPCSStandards/PHP_CodeSniffer#979

Merged

rodrigoprimo mentioned this pull request Mar 5, 2026

Generic/LowerCaseKeyword: remove unnecessary use of prepareForOutput() PHPCSStandards/PHP_CodeSniffer#1379

Merged

12 tasks

Conversation

gmponos commented Feb 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmponos commented Feb 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmponos commented May 9, 2017

Uh oh!

jrfnl commented May 9, 2017

Uh oh!

gmponos commented Jul 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrfnl commented Jul 23, 2017

Uh oh!

gmponos commented Jul 23, 2017

Uh oh!

gmponos commented Jul 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmponos commented Nov 29, 2017

Uh oh!

gsherwood commented Mar 22, 2018

Uh oh!

gsherwood commented Mar 22, 2018

Uh oh!

jrfnl commented Mar 22, 2018

Uh oh!

gsherwood commented Mar 22, 2018

Uh oh!

gmponos commented Mar 22, 2018

Uh oh!

jrfnl commented Mar 22, 2018

Uh oh!

Reviewers

Assignees

Labels