Commit pre-hook misbehaving

[jim-parry]

The recently added PHPCBF pre-commit hook is great to a point, and then it isn't.

Sample output:

PHPCBF RESULT SUMMARY
----------------------------------------------------------------------
FILE FIXED REMAINING
----------------------------------------------------------------------
...iter4/tests/system/Router/RouteCollectionTest.php FAILED TO FIX
...s/CodeIgniter4/tests/system/Router/RouterTest.php FAILED TO FIX
----------------------------------------------------------------------
A TOTAL OF 390 ERRORS WERE FIXED IN 2 FILES
----------------------------------------------------------------------
PHPCBF FAILED TO FIX 2 FILES
----------------------------------------------------------------------

Time: 15.68 secs; Memory: 24Mb


Fix the error before commit.

You will notice the clear indication of just what needs fixing? Me neither :(
I encounter this issue with mods made to tests/system/Router/RouterTest.php.
I can comment out everything, and then uncomment it one method at a time, and all goes well until the next attempt at a commit, and the above re-appears.
PHPCBF is re-formatting the code, and not per our guidelines, as far as arrays go. I can live with that, sorta, but I can't live without the ability to commit.
Anyone else seen this?
The code reformatted by PHPCBF that produces the above is attached.
RouterTest.php.zip

[natanfelles]

Maybe to add phpcs after.

For this file: phpcs --standard=CodeIgniter4 /tmp/RouterTest.php

FILE: /tmp/RouterTest.php
-----------------------------------------------------------------------------------------
FOUND 38 ERRORS AFFECTING 35 LINES
-----------------------------------------------------------------------------------------
 2 | ERROR | [ ] Missing file doc comment
 6 | ERROR | [ ] Missing class doc comment
 6 | ERROR | [ ] Missing doc comment for class RouterTest
 9 | ERROR | [ ] Missing short description in doc comment
 15 | ERROR | [ ] Doc comment short description must start with a capital letter
 17 | ERROR | [ ] Content missing for @var tag in member variable comment
 21 | ERROR | [ ] Missing doc comment for function setUp()
 47 | ERROR | [ ] You must use "/**" style comments for a function comment
 53 | ERROR | [ ] You must use "/**" style comments for a function comment
 65 | ERROR | [ ] You must use "/**" style comments for a function comment
 77 | ERROR | [ ] You must use "/**" style comments for a function comment
 89 | ERROR | [ ] You must use "/**" style comments for a function comment
 101 | ERROR | [ ] You must use "/**" style comments for a function comment
 113 | ERROR | [ ] You must use "/**" style comments for a function comment
 125 | ERROR | [ ] You must use "/**" style comments for a function comment
 137 | ERROR | [ ] Missing short description in doc comment
 139 | ERROR | [ ] Missing @return tag in function comment
 152 | ERROR | [ ] You must use "/**" style comments for a function comment
 168 | ERROR | [ ] You must use "/**" style comments for a function comment
 180 | ERROR | [ ] You must use "/**" style comments for a function comment
 192 | ERROR | [ ] You must use "/**" style comments for a function comment
 208 | ERROR | [ ] You must use "/**" style comments for a function comment
 220 | ERROR | [ ] You must use "/**" style comments for a function comment
 232 | ERROR | [ ] You must use "/**" style comments for a function comment
 244 | ERROR | [ ] You must use "/**" style comments for a function comment
 249 | ERROR | [x] Array key not aligned correctly; expected 0.75 tab but found 2
 250 | ERROR | [x] Array key not aligned correctly; expected 0.75 tab but found 2
 251 | ERROR | [x] Closing parenthesis not aligned correctly; expected 1 tab but found 3
 255 | ERROR | [x] Line indented incorrectly; expected at least 2 tabs, found 0
 256 | ERROR | [x] Line indented incorrectly; expected at least 2 tabs, found 0
 257 | ERROR | [x] Tabs must be used to indent lines; spaces are not allowed
 257 | ERROR | [x] Line indented incorrectly; expected at least 3 tabs, found 1
 257 | ERROR | [x] Closing parenthesis not aligned correctly; expected 1 tab but found 1
 266 | ERROR | [ ] Missing doc comment for function testRouteWorksWithFilters()
 287 | ERROR | [ ] Missing short description in doc comment
 289 | ERROR | [ ] Missing @return tag in function comment
 320 | ERROR | [ ] Missing short description in doc comment
 322 | ERROR | [ ] Missing @return tag in function comment
-----------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 8 MARKED SNIFF VIOLATIONS AUTOMATICALLY
-----------------------------------------------------------------------------------------

Time: 138ms; Memory: 8Mb

It say "PHPCBF CAN FIX THE 8 MARKED SNIFF VIOLATIONS AUTOMATICALLY", but can not (here).

[jim-parry]

I have tried a combination of phpcs & phpcbf, with results along the lines you show.
The problem is that no matter what I do, phpcbf ends up "giving up" without an indication why.
Running phpcs again reports the same things as before, namely some things that can be fixed & some that can't.
The ones that can't seem petty snd should not be showstoppers, preventing a commit.

It seems to me that phpcs/cbf are getting confused with the braced nesting of arrays in this particular test class, and giving up. There were another 18 classes that were "fixed" without question.

[jim-parry]

I have found a single method in each of the "offending" classes that can be commented out to force a "pass"... RouterTest:testMatchedRouteOptions() and RouteColelctionTest::testRoutesOptions().
I am holding my nose at the thought of doing this, but it won't block progress. This issue remains an issue for now, and will need to be revisited.

[jim-parry]

An anomaly that might be affecting things?
RouteCollectionInterface::add($from,$to) but
RouteCollection::add($from,$to,$options).
Different signatures, hmmmmm.

[jim-parry]

Workaround for RouterTest is to declare the options before adding a new rule to the route collection.
It results in the following ugliness (reformatted by PHPCBF):

		$options = [
			'as' => 'login',
			'foo' => 'baz',
		];
		$this->collection->add('foo', function () {
		}, $options);
		$options = [
			'as' => 'admin',
			'foo' => 'bar',
		];
		$this->collection->add(
				'baz', function () {
				}, $options
		);

This is a workaround, not a solution, and smells bad :(

[jim-parry]

Another example of a suspect configuration inside PHPCBF ... it is inappropriately aligning subesquent assignment statements on the = sign, violating any reasonable line length rule. Example:

	public function testBasicHandle()
	{
		$config = new LoggerConfig();
		$config->path = $this->start . 'charlie/';
		$config->handlers['Tests\Support\Log\Handlers\TestHandler']['handles'] = ['critical'];
		$logger = new MockFileHandler($config->handlers['Tests\Support\Log\Handlers\TestHandler']);
		$logger->setDateFormat('Y-m-d H:i:s:u');
		$this->assertTrue($logger->handle('warning', 'This is a test log'));
	}

[lonnieezell]

Good points - and something that needs to be addressed in the CodeIgniter4-Standard repo. I think the last one is in our code style guide, but in situations like that it can cause issues.

I tried the other day to go through and run the fix on everything in the system, which caused a few errors but, unfortunately, haven't had the time to go back and see what was causing it. Something in the database libs.

Should we revert the auto-fix on this for the time being?

[jim-parry]

I have no problem with autofix being on or off - it will get sorted out in the long run, by touching everythng and letting it do its thing. I do like the idea of clean code being forced for commits - we just need to make sure that the "cleanliness" is correct :)

I see two problems to address:

1. PHPCBF gets confused by complicated syntax (the first problem reported above). Worse, it just says "fix the code" without any guidance as to what offends it.
2. the coding standard could be a bit off (eg alignment of assignment operators) or PHPCBF could be mis-applying such rules or over-zeaslously applying them.

Even if PHPCBF's syntax checking doesn't get fixed (re #1), as long as we know about it, we can work around that. We might need some directions in the admin or contributor docs about working around such issues, but that feels like overkill.

Bottom line, after a long-winded think - I would leave the autofix on. I think it is something that contributors should get used to, and we will need to work on updating or tweaking the standard (not a top priority).

[jim-parry]

Punting this over to the coding-standard repo as two bugs