Fix PHPCS issues

Just following on this one.

I've taken a pass at resolving these in MR 62, the vast majority of these were fixed by PHPCBF - the remainder is nearly all docblocks and removing commented out lines, the exception being the removal of strict config schema opt-outs from functional tests.

There is a little bit of overlap with #3357391 for things such as dependency injections, because PHPCS is run using DrupalPractice which is reporting DrupalPractice.Objects.GlobalDrupal.GlobalDrupal and DrupalPractice.Objects.GlobalFunction.GlobalFunction as warnings - but only changes necessary to get PHPCS to pass have been made here.

Thanks for pointing out @ivnish, I've now fixed those cspell issues.

Hi, when I run phpcs on MR!62 I am currently getting 3 errors .. see below..

phpcs --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,info,txt,md,yml web/modules/contrib/backup_migrate

FILE: /home/m/Documents/Issues/backup-migrate-11-3564661/web/modules/contrib/backup_migrate/src/Core/Config/README.md
---------------------------------------------------------------------------------------------------------------------
FOUND 3 ERRORS AND 4 WARNINGS AFFECTING 7 LINES
---------------------------------------------------------------------------------------------------------------------
1 | ERROR | [x] Missing file doc comment
3 | WARNING | [ ] Line exceeds 80 characters; contains 591 characters
5 | WARNING | [ ] Line exceeds 80 characters; contains 388 characters
8 | WARNING | [ ] Line exceeds 80 characters; contains 171 characters
36 | WARNING | [ ] Line exceeds 80 characters; contains 96 characters
56 | ERROR | [x] Concat operator must be surrounded by a single space
58 | ERROR | [x] list(...) is forbidden, use [...] instead.
---------------------------------------------------------------------------------------------------------------------

Hi @schillerm - PHPCS on the CI pipeline passes successfully: https://git.drupalcode.org/issue/backup_migrate-3564661/-/jobs/10497128 - this job uses the drupal-contrib-project standard from the Drupal GitLab Template's default phpcs.xml.dist.

The reason you're seeing failures locally appears to be that you are using a different standard and running PHPCS against the README.md file.

I have reviewed MR and the latest CI pipeline run. The phpcs job now passes successfully with zero coding standards errors or warnings, meaning the primary objective of this issue is resolved.

The overall pipeline failure appears to be caused by other jobs (such as the recent cspell changes or unrelated test failures). Since the PHPCS issues themselves are fully addressed here, those remaining pipeline errors can be broken out and resolved in a separate issue. Ready for RTBC , keeping status needs review for maintainer approval.

Thanks @ivnish, looks like daily_schedule.yml sets keep: '' as a string, while the schema treats keep as an integer.

I've had a look into this and it's not an issue that has been introduced here. It looks like removing strictConfigSchema = FALSE (which was skipping the config schema validation test) has exposed an existing issue:

• 386dcee on 2016-03-11 added the schedule schema with keep as type: integer.
• e65d7a4 on 2020-09-01 introduced AdminFunctionalityTest with strict schema disabled.
• fbd2f1b on 2022-12-10 added the installed daily schedule config with keep: ''.

and then in this branch, strict schema has been enabled again to get PHPCS to pass, exposing the mismatch.

Would you like me to take a pass at making the schedule config/form save keep as an integer on this branch, or would you prefer to treat this as a separate issue?

I believe that's because on that branch AdminFunctionalityTest.php still has the test bypass:

protected $strictConfigSchema = FALSE;

So the issue is obfuscated on the PHPStan branch.

It's only been removed on this branch (to get PHPCS to pass) - thus exposing the issue.

Hi kieran.cott, yes you are right I was missing phpcs.xml. Re ran locally with that phpcs.xml file and no phpcs errors now. Sorry about that.

@kieran.cott could we fix the schedule config/form issue in this MR? please let us know, I can look on it as well.

Thanks @danrod.

I've stored schedule retention as integer 0 for “keep all backups”, completed source config schemas for saved name values, and extended settings profile schema for notification settings.

Form submissions have also been normalised before save: schedule retention and settings profile checkbox values have been cast to the expected scalar types. The related functional test data has been corrected so timestamp format is submitted to timestamp_format.

I've also updated the the Drupal MySQL source to call the up-to-date getConnection() and readSqlCommand() methods.

PHPUnit CI pipeline output now exactly matches that of the 5.1.x branch:

• 5.1.x: https://git.drupalcode.org/project/backup_migrate/-/jobs/10539243
• MR 62: https://git.drupalcode.org/issue/backup_migrate-3564661/-/pipelines/863683

Thank you @kieran.cott

I ran the tests locally and got no failures, I will do some manual tests before merging this.

I did some manual testing and the functionality works as expected, I'm setting this to RTBC, but I'm not merging yet, just in case anyone wants to test this MR as well.

Hi, I just pulled the latest commits and ran phpcs again, no errors showing up. Seems fine to me.

Thanks @schillerm , merging this now.

Merged, thanks a lot to everyone involved.