Skip to content

Conversation

@westonruter
Copy link
Contributor

@westonruter westonruter commented Sep 28, 2024

Submission Review Guidelines:

Changes proposed in this Pull Request:

I encountered a fatal error when accessing the Coming Soon page on a site that is also running the Optimization Detective plugin. The reason is that Optimization Detective adds a template_include filter the same as WooCommerce does, but Optimization Detective adds string type hint for the callback function since the filter defines the $template param as a string. Nevertheless, ComingSoonRequestHandler::handle_template_include() seems to disregard this by returning string|null. The fix seems to simply return an empty string instead of null. I've also fixed this in Optimization Detective by removing the type hint in WordPress/performance#1565, but it could be a problem with other plugins as well.

How to test the changes in this Pull Request:

Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:

  1. Enable the Coming Soon page
  2. Activate the Optimization Detective plugin
  3. Attempt to access the Shop page while being logged-out

Changelog entry

  • Automatically create a changelog entry from the details below.
  • This Pull Request does not require a changelog entry. (Comment required below)
Changelog Entry Details

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

Return an empty string from template_include filter instead of null to avoid PHP fatal error with conflicting plugins using strict types.

Changelog Entry Comment

Comment

@github-actions github-actions bot added plugin: woocommerce Issues related to the WooCommerce Core plugin. type: community contribution labels Sep 28, 2024
@woocommercebot woocommercebot requested review from a team and albarin and removed request for a team September 28, 2024 23:32
@github-actions
Copy link
Contributor

Hi ,

Apart from reviewing the code changes, please make sure to review the testing instructions and verify that relevant tests (E2E, Unit, Integration, etc.) have been added or updated as needed.

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

@albarin albarin closed this Sep 30, 2024
@albarin albarin reopened this Sep 30, 2024
Copy link
Contributor

@albarin albarin left a comment

Choose a reason for hiding this comment

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

Thanks @westonruter for your contribution! It looks good!
I've updated the PR description to include the changelog message so that the changelog could be generated.

@albarin albarin merged commit 38a2b5b into woocommerce:trunk Sep 30, 2024
@github-actions github-actions bot added this to the 9.5.0 milestone Sep 30, 2024
@github-actions github-actions bot added the needs: analysis Indicates if the PR requires a PR testing scrub session. label Sep 30, 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 Oct 1, 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 type: community contribution

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants