Automattic / woocommerce-payments

Accept payments via credit card. Manage transactions within WordPress.
https://wordpress.org/plugins/woocommerce-payments/
Other
173 stars 69 forks source link

Fix reserved keyword variable names phpcs reports #8438

Open reykjalin opened 5 months ago

reykjalin commented 5 months ago

Description

This is a follow up to https://github.com/Automattic/woocommerce-payments/pull/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='Universal.NamingConventions.NoReservedKeywordParameterNames'
Results in a list of issues in these files (also a command to get just the list of files) ```sh ./vendor/bin/phpcs --standard=phpcs.xml.dist $(git ls-files | grep '.php$') --sniffs='Universal.NamingConventions.NoReservedKeywordParameterNames' | rg 'FILE' | sed 's/FILE: \/path\/to\/wcpay\/repo\/parent\/woocommerce-payments\//* ' ``` * `includes/fraud-prevention/models/class-check.php` * `includes/multi-currency/Compatibility/WooCommerceUPS.php` * `includes/fraud-prevention/models/class-rule.php` * `tests/unit/helpers/class-wc-mock-wc-data-store.php` * `includes/core/server/request/class-list-deposits.php` * `includes/multi-currency/AdminNotices.php` * `includes/core/server/request/class-list-disputes.php` * `includes/core/server/request/class-list-documents.php` * `includes/core/server/request/class-list-transactions.php` * `includes/fraud-prevention/class-fraud-risk-tools.php` * `includes/multi-currency/Analytics.php` * `src/Internal/Service/SessionService.php` * `includes/multi-currency/Compatibility/WooCommerceBookings.php` * `tests/unit/test-class-wc-payments-features.php` * `includes/multi-currency/Compatibility/WooCommerceFedEx.php` * `includes/multi-currency/Compatibility/WooCommerceNameYourPrice.php` * `tests/unit/helpers/class-wc-helper-subscription.php` * `tests/unit/helpers/class-wc-helper-subscriptions.php` * `includes/multi-currency/Compatibility/WooCommerceProductAddOns.php` * `includes/multi-currency/Compatibility/WooCommerceSubscriptions.php` * `includes/wc-payment-api/class-wc-payments-api-client.php` * `includes/class-wc-payments-payment-request-button-handler.php` * `includes/class-wc-payments-utils.php` * `includes/class-wc-payments-webhook-processing-service.php` * `tests/unit/test-class-woopay-tracker.php` * `tests/unit/wc-payment-api/test-class-wc-payments-api-client.php`

Many of these seem legit, where we're literally operating on a "string" or "object", warranting those variable names, but I agree that in general it's better to avoid those names. With the number of files where this sniff is reporting issues there's bound to be some where the variable names should be fixed.

I don't think we should ignore this rule project-wide, so ignores in place may be more appropriate, but I wouldn't rule out a project-wide ignore if all the above are legitimate cases where a variable name is the same as a keyword is warranted.

Acceptance criteria

No more issues reported by phpcs.

### Tasks
- [ ] https://github.com/Automattic/woocommerce-payments/issues/8537
- [ ] https://github.com/Automattic/woocommerce-payments/issues/8538
- [ ] https://github.com/Automattic/woocommerce-payments/issues/8539
- [ ] https://github.com/Automattic/woocommerce-payments/issues/8540
- [ ] https://github.com/Automattic/woocommerce-payments/issues/8541
- [ ] https://github.com/Automattic/woocommerce-payments/issues/8542
- [ ] https://github.com/Automattic/woocommerce-payments/issues/8543
- [ ] https://github.com/Automattic/woocommerce-payments/issues/8544
- [ ] https://github.com/Automattic/woocommerce-payments/issues/8545
reykjalin commented 5 months ago

@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 🙏

vbelolapotkov commented 5 months ago

@reykjalin thanks for splitting them up! Most of the issues look good. I've commented on individual ones, where I had doubts.