Problem/Motivation

The PHPCS coding standards lint job is not checking the php scripts stored in /scripts/ and specifically not checking run-tests.sh.

The reason is that the core phpcs.xml.dist file does not include sh in the list of extensions, probably because in general .sh files are bash scripts not php. But it seems there was an attempt to include specific .sh files in the check. I don't know if this ever worked, but it certainly does nothing now, at least locally for me, because the list in "extensions" overrides any exceptions here.

There is currently a lot of activity relating to run-tests.sh - this problem was discovered in #3515347-57: Reduce run-tests.sh complexity in spawning subprocesses #57 but there are many other issues - see #2626986: [meta] Improvements to run-tests.sh so it would be very helpful if the PHPCS coding standards could be run. I don't know if PHPStan is being run on these php scripts, but that would be very useful too.

Proposed resolution

Add a PhpCFS filter ala https://stackoverflow.com/questions/47415146/how-can-php-codesniffer-che... so we can ensure any file listed in phpcs.xml.dist is scanned even if it does not match an extension in the list. This is how PHPCS 2 worked.

Remaining tasks

None

User interface changes

None

Introduced terminology

None

API changes

None

Data model changes

None

Release notes snippet

N/a

Issue fork drupal-3568959

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

jonathan1055 created an issue. See original summary.

jonathan1055’s picture

jonathan1055 commented
Issue summary: View changes

I know that there is a long-standing issue #655178: [PP-2] Rename run-tests.sh to run-tests.php but I'm not suggesting we push for that either. The scripts can stay exactly like now, we just need them to be checked in the linting jobs.

mondrake’s picture

mondrake
Location 🇮🇹
commented

AFAICS PHPStan does do checking of run-tests.sh.

EDIT: yep, there are even baselined errors 

$ignoreErrors[] = [
	'message' => '#^Function simpletest_script_execute_batch\\(\\) has no return type specified\\.$#',
	'identifier' => 'missingType.return',
	'count' => 1,
	'path' => __DIR__ . '/scripts/run-tests.sh',
];
$ignoreErrors[] = [
	'message' => '#^Function simpletest_script_run_phpunit\\(\\) has no return type specified\\.$#',
	'identifier' => 'missingType.return',
	'count' => 1,
	'path' => __DIR__ . '/scripts/run-tests.sh',
];
$ignoreErrors[] = [
	'message' => '#^Function simpletest_script_setup_test_run_results_storage\\(\\) has no return type specified\\.$#',
	'identifier' => 'missingType.return',
	'count' => 1,
	'path' => __DIR__ . '/scripts/run-tests.sh',
];
mondrake’s picture

mondrake
Location 🇮🇹
commented
Issue tags: +run-tests.sh

jonathan1055 opened merge request !14471

jonathan1055’s picture

jonathan1055 commented

The core phpcs.xml.dist has the following, specifically naming four scripts:

  <file>.</file>
  <file>../composer</file>
  <file>scripts/password-hash.sh</file>
  <file>scripts/rebuild_token_calculator.sh</file>
  <file>scripts/run-tests.sh</file>
  <file>scripts/update-countries.sh</file>

Adding a second call to phpcs using --extensions=sh and the job checks five files. We see:

----------------------------------------------------------------------
FILE                                                  ERRORS  WARNINGS
----------------------------------------------------------------------
/builds/core/scripts/password-hash.sh                 2       0
/builds/core/scripts/rebuild_token_calculator.sh      0       0
/builds/core/scripts/run-tests.sh                     32      4
/builds/core/scripts/update-countries.sh              0       0
/builds/core/scripts/dev/commit-code-check.sh         0       1
----------------------------------------------------------------------
A TOTAL OF 34 ERRORS AND 5 WARNINGS WERE FOUND IN 5 FILES
----------------------------------------------------------------------

So the <file> list for those four scripts is being ignored probably because the <file>.</file> means 'check everything here and below'. That list of files can probably be deleted from phpcs.xml.dist

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Issue summary: View changes
Status: Active » Needs review

Copying the fix from the duplicate I filed - #3589520: Since we moved to PHPCS3 run-tests.sh amongst other things is not scanned.

jonathan1055’s picture

jonathan1055 commented

Thanks for doing this. Looks like you have solved it.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Thanks @jonathan1055, fancy RTBC-ing this then?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Re #6 I don't think PHPCS should be scanning builds/core/scripts/dev/commit-code-check.sh - it's not PHP :)

smustgrave’s picture

smustgrave commented

Left one question on the MR but don't mind marking it.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Thanks alexpott

jonathan1055’s picture

jonathan1055 commented

I know that the phpcs job ends green but there is no actual evidence that the file extra files are being checked. Would it be worth doing a temporary change to run it with the -v flag to show each file that has been checked? Is there a way to pass in extra parameters via a manual pipeline run, like we can in contrib pipelines?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@jonathan1055 over on the duplicate MR there is evidence https://git.drupalcode.org/project/drupal/-/merge_requests/15731/pipelines - plus it is quite simple to prove locally.

jonathan1055’s picture

jonathan1055 commented

Thanks for the link to https://git.drupalcode.org/project/drupal/-/jobs/9767126#L398
Yes I was just being lazy and not gettting this new work locally. I have now done that, and get

PHP CODE SNIFFER REPORT SUMMARY
----------------------------------------------------------------------
FILE                                                  ERRORS  WARNINGS
----------------------------------------------------------------------
core/scripts/db-tools.php                             0       0
core/scripts/drupal                                   0       0
core/scripts/dump-database-d8-mysql.php               0       0
core/scripts/generate-proxy-class.php                 0       0
core/scripts/PackageManagerFixtureCreator.php         0       0
core/scripts/password-hash.sh                         0       0
core/scripts/rebuild_token_calculator.sh              0       0
core/scripts/run-tests.sh                             0       0
core/scripts/test-site.php                            0       0
core/scripts/update-countries.sh                      0       0
----------------------------------------------------------------------
A TOTAL OF 0 ERRORS AND 0 WARNINGS WERE FOUND IN 10 FILES
----------------------------------------------------------------------

So all's good. It's a really nice use of a custom phpcs filter. I did not know about them before, so thank you for that, too.

longwave made their first commit to this issue’s fork.

  • longwave committed 14676bf6 on main 
    ci: #3568959 PHPCS ignores the attempt to check run-tests.sh

By:...
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed 14676bf640b to main. Thanks!

Doesn't backport cleanly, don't think it's worth the effort as run-tests.sh will only really change in main going forward.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

longwave closed merge request !14471

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.