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

Apply Rector suggestions for PHP 8.1 #41253

Merged
merged 13 commits into from Feb 19, 2024
Merged

Apply Rector suggestions for PHP 8.1 #41253

merged 13 commits into from Feb 19, 2024

Conversation

asumaran
Copy link
Contributor

@asumaran asumaran commented Nov 6, 2023

Changes proposed in this Pull Request:

Applied suggestions based on Rector's static analysis for PHP 8.1.

This is a continuation of #40801

Ref: D128479-code

How to test the changes in this Pull Request:

  • plugins/woocommerce/includes/admin/class-wc-admin-importers.php
  • plugins/woocommerce/includes/admin/class-wc-admin-webhooks.php
    • Go to WooCommerce > Settings > Advanced > Webhooks
    • Create a webhook.
    • Click “Delete permanently”
    • It should delete the webhook without issues.
  • plugins/woocommerce/includes/admin/helper/class-wc-helper.php
    • Add the following snippet using the Code Snippets plugin:
      add_filter(
      'pre_option_woocommerce_feature_marketplace_enabled',
      fn() => 'no'
      );
    • Go to https://your.site/wp-admin/admin.php?page=wc-addons
    • Go to “My Subscriptions”
    • Click “Connect” to connect using your A8C account.
    • It should show the subscriptions page without issues.
  • plugins/woocommerce/includes/admin/importers/class-wc-tax-rate-importer.php
    • Go to WooCommerce > Settings. Check “Enable tax rates and calculations” then save changes.
    • Go to WooCommerce > Settings > Tax > Standard rates.
    • Click “Insert row” then “Export CSV”
    • Click “Import CSV” and upload the exported file. It should import taxes without issues.
  • plugins/woocommerce/includes/admin/importers/views/html-csv-import-done.php
    • Go to Products. Having at least on product created, then click “Export”. Then click “Generate CV”
    • Go to Products. Click “Import” then upload the .csv file from the previous step.
    • Run the importer. It should finish without issues. Clicking “View import log” should also work.
    • Notice the product was probably skipped because it existed already. That’s expected.
  • plugins/woocommerce/includes/admin/list-tables/class-wc-admin-list-table-coupons.php
    • Go to Marketing > Coupons.
    • Add a new coupon. Click “Usage restriction”. Under “Products” choose an existing product then click “publish”
    • Go to Marketing > Coupons. You should see the table of coupons. The Product ID should be in the “Product IDs” column.
  • plugins/woocommerce/includes/admin/meta-boxes/views/html-product-data-variations.php
    • Go to Products > Add New.
    • In the product type dropdown select “Variable product”
    • Click the “Variations” tab. Should be able to see the empty view without issues.
  • plugins/woocommerce/includes/admin/reports/class-wc-report-sales-by-category.php
    • Create a product. Add a price and assign a category to the product.
    • Create an order. Set status as “Completed”. Add product from previous step as item.
    • Go to WooCommerce > Reports > Sales by Category.
    • Choose the category from the first step. Click “show”
    • Should be able to see the chart without issues.
  • plugins/woocommerce/includes/admin/settings/class-wc-settings-tax.php
    • The tax tables should be visible, can insert or remove tax rates without issues.
    • The method save_tax_classes is not used anymore. Tax are saved via ajax now.
  • plugins/woocommerce/includes/admin/views/html-admin-page-status-report.php
    • Go to WooCommerce > Status. The System status report should be visible without issues.
  • plugins/woocommerce/includes/class-wc-ajax.php
    • Go to Products > Categories.
    • Sorting categories using the ≡ button should work as expected.
  • plugins/woocommerce/includes/class-wc-deprecated-action-hooks.php
    • No need to test changes in this file as they are deprecated hooks that were never called anyway.
  • plugins/woocommerce/includes/class-wc-geo-ip.php
    • No need to test this as changes in this file are insignificant. Will never break the app.

Changelog entry

  • Automatically create a changelog entry from the details below.

Significance

  • Patch
  • Minor
  • Major

Type

  • Fix - Fixes an existing bug
  • Add - Adds functionality
  • Update - Update existing functionality
  • Dev - Development related task
  • Tweak - A minor adjustment to the codebase
  • Performance - Address performance issues
  • Enhancement - Improvement to existing functionality

Message

Apply Rector suggestions for PHP 8.1

Comment

@asumaran asumaran added the plugin: woocommerce Issues related to the WooCommerce Core plugin. label Nov 6, 2023
@asumaran asumaran self-assigned this Nov 6, 2023
Copy link
Contributor

github-actions bot commented Nov 6, 2023

Test Results Summary

Commit SHA: ea65150

Test 🧪Passed ✅Failed 🚨Broken 🚧Skipped ⏭️Unknown ❔Total 📊Duration ⏱️
API Tests25900202610m 37s
E2E Tests324001203367m 11s

To view the full API test report, click here.
To view the full E2E test report, click here.
To view all test reports, visit the WooCommerce Test Reports Dashboard.

@asumaran asumaran changed the base branch from trunk to release/8.2 November 6, 2023 21:44
@asumaran asumaran changed the base branch from release/8.2 to trunk November 6, 2023 21:45
@asumaran asumaran changed the title Update to support PHP 8.1 Apply Rector suggestions for PHP 8.1 Nov 14, 2023
Copy link
Contributor

github-actions bot commented Nov 14, 2023

Hi @coreymckrill,

Apart from reviewing the code changes, please make sure to review the testing instructions as well.

You can follow this guide to find out what good testing instructions should look like:
https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

@asumaran asumaran marked this pull request as ready for review November 15, 2023 23:19
Copy link
Contributor

@prettyboymp prettyboymp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@asumaran, thanks for working on this. I've made some notes on the changes. A lot of these are false positives that should be fine with PHP8 given the context where they are used. I think it's better to keep the code clean here rather than add the extra checks.

plugins/woocommerce/includes/class-wc-ajax.php Outdated Show resolved Hide resolved
plugins/woocommerce/includes/class-wc-discounts.php Outdated Show resolved Hide resolved
plugins/woocommerce/includes/class-wc-discounts.php Outdated Show resolved Hide resolved
plugins/woocommerce/includes/class-wc-form-handler.php Outdated Show resolved Hide resolved
@asumaran asumaran force-pushed the as-update-to-php-81 branch 4 times, most recently from 84cf710 to 318fe82 Compare November 27, 2023 21:12
@asumaran
Copy link
Contributor Author

asumaran commented Nov 28, 2023

@prettyboymp E2E tests seem to fail for unrelated reasons. I've tried running them locally and the results are inconsistent.

Edit:

I'm investigating if failing tests are actually legit

@asumaran
Copy link
Contributor Author

@prettyboymp I've fixed all test and lint issues. I think this is ready for a second look.

Copy link
Contributor

@prettyboymp prettyboymp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good. May need to change the changelog entry though.

@asumaran asumaran requested review from a team and coreymckrill and removed request for a team January 17, 2024 23:05
@coreymckrill
Copy link
Collaborator

Looks like CI is stuck. I'm going to close and re-open this to see if that fixes it.

Copy link
Collaborator

@coreymckrill coreymckrill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me. is_countable is available starting in PHP 7.3, so meets WC's minimum PHP version requirement of 7.4.

@asumaran
Copy link
Contributor Author

@coreymckrill can we merge this PR?

@coreymckrill
Copy link
Collaborator

Looks like we still need a new approval from @prettyboymp since there were requested changes.

@asumaran
Copy link
Contributor Author

@coreymckrill, @prettyboymp can you review again? I've already changed the changelog entry.

@asumaran asumaran marked this pull request as draft January 23, 2024 18:27
@asumaran asumaran marked this pull request as ready for review February 16, 2024 21:25
@asumaran
Copy link
Contributor Author

Looks like we still need a new approval from @prettyboymp since there were requested changes.

@coreymckrill can we merge this PR? All suggestions made by @prettyboymp have been addressed.

@coreymckrill coreymckrill dismissed prettyboymp’s stale review February 19, 2024 18:34

Requested changes have been made.

@coreymckrill coreymckrill merged commit 0407569 into trunk Feb 19, 2024
36 checks passed
@coreymckrill coreymckrill deleted the as-update-to-php-81 branch February 19, 2024 20:29
@github-actions github-actions bot added this to the 8.7.0 milestone Feb 19, 2024
@github-actions github-actions bot added the needs: analysis Indicates if the PR requires a PR testing scrub session. label Feb 19, 2024
@Stojdza Stojdza added needs: external testing Indicates if the PR requires further testing conducted by testers external to the development team. status: analysis complete Indicates if a PR has been analysed by Solaris and removed needs: analysis Indicates if the PR requires a PR testing scrub session. labels Feb 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs: external testing Indicates if the PR requires further testing conducted by testers external to the development team. plugin: woocommerce Issues related to the WooCommerce Core plugin. status: analysis complete Indicates if a PR has been analysed by Solaris
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants