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 WooCommerce hook comment phpcs reports #8437

Open
10 tasks
reykjalin opened this issue Mar 21, 2024 · 1 comment
Open
10 tasks

Fix WooCommerce hook comment phpcs reports #8437

reykjalin opened this issue Mar 21, 2024 · 1 comment
Labels
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: documentation This issue is a request for better documentation. 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='WooCommerce.Commenting.CommentHooks'
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='WooCommerce.Commenting.CommentHooks' | rg 'FILE' | sed 's/FILE: \/path\/to\/wcpay\/repo\/parent\/woocommerce-payments\//* '
  • templates/emails/plain/customer-ipp-receipt.php
  • tests/unit/core/server/request/test-class-core-create-and-confirm-intention-request.php
  • tests/unit/woopay/services/test-checkout-service.php
  • src/Internal/Proxy/HooksProxy.php
  • tests/unit/core/server/request/test-class-core-create-intention-request.php
  • includes/class-wc-payments-checkout.php
  • tests/unit/admin/test-class-wc-payments-admin-sections-overwrite.php
  • includes/multi-currency/CurrencySwitcherWidget.php
  • includes/express-checkout/class-wc-payments-express-checkout-button-helper.php
  • includes/class-wc-payments-dependency-service.php
  • tests/unit/core/server/request/test-class-core-request.php
  • includes/class-wc-payments-features.php
  • includes/admin/class-wc-rest-payments-settings-controller.php
  • includes/multi-currency/Geolocation.php
  • tests/unit/helpers/class-wc-helper-deposit-product-manager.php
  • src/Internal/Payment/Router.php
  • includes/multi-currency/Analytics.php
  • src/Internal/Service/OrderService.php
  • includes/class-wc-payments-fraud-service.php
  • tests/unit/core/server/request/test-class-get-request.php
  • templates/emails/customer-ipp-receipt.php
  • includes/multi-currency/Compatibility.php
  • tests/unit/src/Internal/Proxy/HooksProxyTest.php
  • tests/unit/helpers/class-wc-helper-product.php
  • includes/core/class-mode.php
  • includes/multi-currency/Compatibility/WooCommerceProductAddOns.php
  • tests/unit/multi-currency/compatibility/test-class-woocommerce-subscriptions.php
  • includes/class-database-cache.php
  • includes/core/server/class-request.php
  • includes/multi-currency/MultiCurrency.php
  • includes/wc-payment-api/class-wc-payments-api-client.php
  • includes/multi-currency/StorefrontIntegration.php
  • tests/unit/test-class-wc-payment-gateway-wcpay-process-payment.php
  • tests/unit/test-class-wc-payment-gateway-wcpay-subscriptions-payment-method-order-note.php
  • includes/admin/class-wc-payments-admin.php
  • tests/unit/subscriptions/test-class-wc-payments-subscription-migration-log-handler.php
  • includes/class-wc-payments-payment-request-button-handler.php
  • includes/admin/class-wc-rest-payments-orders-controller.php
  • includes/class-wc-payment-gateway-wcpay.php
  • includes/class-wc-payments-webhook-processing-service.php
  • includes/class-wc-payments-woopay-button-handler.php
  • tests/unit/test-class-wc-payments.php
  • includes/class-wc-payments-account.php
  • includes/class-wc-payments.php

I think this matches well with some documentation effort on the hooks available in WooPayments so this is probably a good issue to address across the codebase.

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

  1. focus: woopay priority: low type: documentation type: technical debt
  2. focus: payouts priority: low type: documentation type: technical debt
  3. focus: subscriptions priority: low type: documentation type: technical debt
  4. focus: misc or unknown priority: low type: documentation type: technical debt
  5. focus: multi-currency priority: low type: documentation type: technical debt
  6. focus: payments acceptance & processing priority: low type: documentation type: technical debt
  7. focus: checkout payments priority: low type: documentation type: technical debt
  8. focus: account lifecycle priority: low type: documentation type: technical debt
  9. focus: architecture priority: low type: documentation type: technical debt
  10. focus: fraud tools priority: low type: documentation 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. type: documentation This issue is a request for better documentation. 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
@vbelolapotkov vbelolapotkov added type: technical debt This issue/PR represents/solves the technical debt of the project. focus: misc or unknown Issues that need to be added to a focus area (aka "needs focus"). and removed category: devops Features and tools supporting dev process. type: developer experience focus: devops Release processes, monitoring, automations, dev tools, CI/CD pipeline labels Mar 26, 2024
@reykjalin
Copy link
Contributor Author

@vbelolapotkov I've split this issue up according to my best interpretation of the focus are labels in the repo. I kept the same priority for all the different issues.

If you could take a quick glance through the issues to make sure nothing is obviously in the wrong focus are that'd be great 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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: documentation This issue is a request for better documentation. 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