Fix phpcs violations + add phpcs to CI#2
Merged
Conversation
phpcbf auto-fixed 36 violations (blank-line spacing on enums and similar). Manual fixes: - src/Client.php: split long URL string into a renterId variable - src/ClientException.php: add a marker comment so the empty class body doesn't trip the empty-lines-around-braces rules - src/Model/Address.php: extract repeated regex/message into class constants, line-wrap the message to stay under 120 chars - test/Unit/Model/AddressTest.php: extract long fixture strings into local variables and the shared message into a constant; reformat the data provider rows so each fits on one line Add a phpcs CI job that runs against vendor/rentpost/coding-standard-php with --warning-severity=0, so warnings (TU API field names that include digits, intentional unused PHPUnit @Depends params) are visible but only errors fail the build. All 66 unit tests pass.
The just-merged coding-standard-php master tightens SlevomatCodingStandard.Classes.ClassStructure so 'public static methods' must come before 'public methods'. phpcbf moved the static methods (fromArray, validationProvider, etc.) above the instance methods across the model and test classes. Pure reorder: every file has equal additions and removals, no logic touched.
… in CI - src/Model/Address.php: addressLine1..4 are TU API field names; renaming them would break the wire contract. Disable the Zend.NamingConventions.ContainsNumbers sniff for this file with a documented phpcs:disable directive. - test/Integration/IntegrationTest.php: testValidateRenterForScreeningRequest and testCreateReport must accept return values from PHPUnit #[Depends] chains, which means some parameters are unavoidably unused. Suppress the Generic.CodeAnalysis.UnusedFunctionParameter sniff for those two methods with explanatory comments on the parameters themselves. - .github/workflows/ci.yml: drop --warning-severity=0 from the phpcs job so CI now fails on either an error or a warning, since neither type should ever reappear.
PHPUnit runs tests in source order, and cancelling the screening request
before testCreateExam puts it in a terminal state in the TU sandbox. TU
then rejects createExam with "IDV not possible as the reports were
already generated" because cancelled is treated as a final report state.
Move cancel to the very end and add a #[Depends('testGetReportsForRenter')]
so it can never run before the report chain even if PHPUnit ever changes
its default ordering.
Cancelling the main screening request after the report chain fails with ScreeningRequestCannotCancel because TU treats a completed SR as terminal. Cancelling before the report chain fails because IDV requires a non- terminal SR. There's no order that makes both flows happy on the same SR. Have testCancelScreeningRequestForRenter create a dedicated, never- completed screening request for the existing renter and cancel that one, so the API endpoint is still exercised without disturbing the report flow.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Brings
src/andtest/into compliance with our shared coding standard, and gates that compliance in CI going forward.Code style fixes
phpcbf(mostly blank-line spacing on enums + the integration test class)src/Client.php: split a 125-char URL into a$renterIdvariablesrc/ClientException.php: add a marker comment so the empty class body satisfies both empty-lines-around-braces rulessrc/Model/Address.php: extract the repeated regex pattern and message into class constants; line-wrap the message under 120 charstest/Unit/Model/AddressTest.php: extract long fixture strings into local variables and the shared error message into a constant; reformat the data provider so every row fits on one lineCI
phpcsjob in.github/workflows/ci.ymlruns againstvendor/rentpost/coding-standard-php/phpcs.xml--warning-severity=0so only errors fail. The 30 remaining warnings are unavoidable: 28 are TU API field names that contain digits (addressLine1–4), and 2 are PHPUnit#[Depends]parameter chains where the params are intentionally unusedTest plan
vendor/bin/phpcs --standard=vendor/rentpost/coding-standard-php/phpcs.xml --warning-severity=0 src/ test/exits 0vendor/bin/phpunit --testsuite unit— 66/66 passgh workflow run ci.yml --ref chore/fix-phpcs-violationsto confirm nothing in the touched code broke real TU API calls