Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix unused parameter phpcs sniffs #8436

Open
1 of 7 tasks
reykjalin opened this issue Mar 21, 2024 · 4 comments
Open
1 of 7 tasks

Fix unused parameter phpcs sniffs #8436

reykjalin opened this issue Mar 21, 2024 · 4 comments
Labels
category: engineering For product engineering, architecture work, tech debt and so on. focus: misc or unknown Issues that need to be added to a focus area (aka "needs focus"). priority: low The issue/PR is low priority—not many people are affected or there’s a workaround, etc. type: technical debt This issue/PR represents/solves the technical debt of the project.

Comments

@reykjalin
Copy link
Contributor

reykjalin commented Mar 21, 2024

Description

This is a follow up to #8415 where there are some issues reported by phpcs that were not a straight-forward fix.

Running

./vendor/bin/phpcs --standard=phpcs.xml.dist $(git ls-files | grep '.php$') --sniffs='VariableAnalysis.CodeAnalysis.VariableAnalysis'
Results in a list of issues in these files (also a command to get just the list of files)
./vendor/bin/phpcs --standard=phpcs.xml.dist $(git ls-files | grep '.php$') --sniffs='VariableAnalysis.CodeAnalysis.VariableAnalysis' | rg 'FILE' | sed 's/FILE: \/path\/to\/wcpay\/repo\/parent\/woocommerce-payments\//* '
  • tests/unit/subscriptions/test-class-wc-payments-subscription-service.php
  • tests/unit/subscriptions/test-class-wc-payments-subscriptions-event-handler.php
  • includes/payment-methods/class-upe-payment-method.php
  • tests/unit/subscriptions/test-class-wc-payments-subscriptions-migrator.php
  • includes/multi-currency/Analytics.php
  • includes/multi-currency/FrontendCurrencies.php
  • tests/unit/test-class-compatibility-service.php
  • src/Internal/Payment/State/AbstractPaymentState.php
  • tests/unit/multi-currency/test-class-multi-currency.php
  • tests/unit/test-class-wc-payments-features.php
  • tests/unit/helpers/class-wc-helper-product-add-ons-helper.php
  • includes/class-wc-payments-incentives-service.php
  • tests/unit/test-class-wc-payment-gateway-wcpay-payment-types.php
  • tests/unit/core/server/request/test-trait-order-info.php
  • tests/unit/helpers/class-wc-helper-subscriptions-synchroniser.php
  • tests/unit/multi-currency/compatibility/test-class-woocommerce-subscriptions.php
  • includes/class-logger.php
  • includes/multi-currency/MultiCurrency.php
  • tests/unit/multi-currency/test-class-analytics.php
  • tests/unit/test-class-wc-payment-gateway-wcpay-process-payment.php
  • includes/wc-payment-api/class-wc-payments-api-client.php
  • includes/admin/class-wc-payments-admin-sections-overwrite.php
  • tests/unit/subscriptions/test-class-wc-payments-invoice-service.php
  • tests/unit/test-class-wc-payment-gateway-wcpay-subscriptions-payment-method-order-note.php
  • tests/unit/subscriptions/test-class-wc-payments-subscription-change-payment-method.php
  • tests/unit/test-class-wc-payment-gateway-wcpay-subscriptions-process-payment.php
  • tests/unit/test-class-wc-payments-utils.php
  • tests/unit/test-class-wc-payment-gateway-wcpay-subscriptions.php
  • includes/class-wc-payments-webhook-processing-service.php
  • includes/class-wc-payments.php
  • includes/class-wc-payments-apple-pay-registration.php
  • tests/unit/test-class-wc-payment-gateway-wcpay.php

These seem to be mostly valid cases. I'd love to see if we can maybe add some rule to phpcs.xml.dist that catches these valid cases, e.g. in filters and hooks.

Alternatively we could just ignore these outright. Either way we should go through all of these issues and either:

  1. Fix the issue, if it exists;
  2. Add ignores in-place via comments; or
  3. Add an ignore to phpcs.xml.dist.

Acceptance criteria

No more issues reported by phpcs.

Tasks

Preview Give feedback
  1. focus: woopay priority: low type: technical debt
  2. focus: architecture priority: low type: technical debt
  3. focus: architecture priority: low type: technical debt
  4. focus: multi-currency priority: low type: technical debt
  5. focus: account lifecycle priority: low type: technical debt
  6. focus: checkout payments priority: low type: technical debt
  7. focus: payments acceptance & processing priority: low type: technical debt
@reykjalin reykjalin added priority: low The issue/PR is low priority—not many people are affected or there’s a workaround, etc. category: devops Features and tools supporting dev process. type: developer experience focus: devops Release processes, monitoring, automations, dev tools, CI/CD pipeline labels Mar 21, 2024
@reykjalin
Copy link
Contributor Author

Maybe https://github.com/sirbrillig/phpcs-variable-analysis/ could be a decent fit here to replace this set of rules?

@vbelolapotkov vbelolapotkov added focus: misc or unknown Issues that need to be added to a focus area (aka "needs focus"). type: technical debt This issue/PR represents/solves the technical debt of the project. category: engineering For product engineering, architecture work, tech debt and so on. and removed focus: devops Release processes, monitoring, automations, dev tools, CI/CD pipeline category: devops Features and tools supporting dev process. type: developer experience labels Mar 26, 2024
@vbelolapotkov
Copy link
Collaborator

@reykjalin this one will need to be split into smaller issues. The same as we did for other recent phpcs issues. Could you please take care of it once you have time?

@reykjalin
Copy link
Contributor Author

Yup, once #8556 has been merged we'll have some better tools and direction for managing this across the codebase so I'll split it then 👍

@reykjalin
Copy link
Contributor Author

@vbelolapotkov I've split this up into what I think are correct focuses, but would appreciate a quick review that this is split up correctly 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: engineering For product engineering, architecture work, tech debt and so on. focus: misc or unknown Issues that need to be added to a focus area (aka "needs focus"). priority: low The issue/PR is low priority—not many people are affected or there’s a workaround, etc. type: technical debt This issue/PR represents/solves the technical debt of the project.
Projects
None yet
Development

No branches or pull requests

2 participants