First-party Woo plugin detection for product customizations#4552
First-party Woo plugin detection for product customizations#4552strangerkir wants to merge 16 commits into
Conversation
| */ | ||
| public function test_all_false_when_no_plugin_is_active(): void { | ||
| $this->plugin_detector->shouldReceive( 'scan' )->once()->andReturn( $this->all_inactive() ); | ||
| $this->logger->shouldNotReceive( 'warning' ); |
There was a problem hiding this comment.
suggestion: Remove this
Reason: Log output should not be tested/expected; when we add or change log output, it should not impact the test
Usual mock for the logger:
// Define the logger with shouldIgnoreMissing in the setUp() method, then forget about it:
$this->logger = Mockery::mock(LoggerInterface::class)->shouldIgnoreMissing();There was a problem hiding this comment.
I'd still keep it tested from a practical point of view. If a 3rd-party API changes and the function no longer exists, this warning is the only way to know that the check itself was a false negative. So I'd say this is an important part of the logic, not just regular logging.
But I agree that testing for a particular string in logs may be too much, so I'm leaving just a warning assertion, without any message checks.
UPD: removed here, this one is useless, as we never examine real 3rd-party functions here. My comment above related to the next warning assertion.
| /** | ||
| * Interface PluginDetectorInterface | ||
| */ | ||
| interface PluginDetectorInterface { |
There was a problem hiding this comment.
suggestion: Delete this interface
Reason: I cannot see any hint that there will be more than one implementation of this interface. Instead of adding it right away, we generally try starting with a single class and add an interface once it's really needed
| /** | ||
| * @inheritDoc | ||
| */ |
There was a problem hiding this comment.
nit: Remove the docblock
Reason: it's 100% noise, adding no value. PhpStorm expands the parent docblock automatically.
Of course, if the interface is removed (as I suggested), move the original docblock here 😉
There was a problem hiding this comment.
Done. The interface was removed, and I only left the file header here, removing the class docblock. It looks like in this project we often prefer file headers, so I followed the same pattern.
| /** | ||
| * Checks whether WooCommerce Subscriptions is active. | ||
| * | ||
| * @return bool | ||
| */ |
There was a problem hiding this comment.
nit: Remove all the docblocks in this file.
Reason: the return value is documented in code. And the function names are excellent, they do not need any explanation
There was a problem hiding this comment.
Agree, this is nothing more than noise. Removed now.
| * @package WooCommerce\PayPalCommerce\Compat\PluginDetector | ||
| */ | ||
|
|
||
| declare(strict_types=1); |
There was a problem hiding this comment.
nit: apply code style rules to this file
Reason: The project style expects spaces around operators and parentheses
Sample:
declare( strict_types = 1 );| /** | ||
| * Class ProductCustomizationDetector | ||
| */ |
There was a problem hiding this comment.
nit: Remove this docblock
😉
| /** | ||
| * @var PluginDetectorInterface | ||
| */ |
There was a problem hiding this comment.
nit: Remove the docblock, and the others that just re-state what's visible in the code
There was a problem hiding this comment.
Removed completely :)
| /** | ||
| * Cached result of $plugin_detector->scan(), since plugin activation | ||
| * cannot change within a request but scan() may be called once per product. | ||
| * | ||
| * @var array<string, bool>|null | ||
| */ |
There was a problem hiding this comment.
note: This @var annotation is good, it this docblock should stay
(just want to point out that this is different/useful in contrast to the comments above)
| 'woocommerce-subscriptions' => $this->is_woocommerce_subscriptions_active(), | ||
| 'woocommerce-gift-cards' => $this->is_woocommerce_gift_cards_active(), | ||
| 'woocommerce-product-bundles' => $this->is_woocommerce_product_bundles_active(), | ||
| 'woocommerce-product-addons' => $this->is_woocommerce_product_addons_active(), | ||
| 'woocommerce-min-max-quantities' => $this->is_woocommerce_min_max_quantities_active(), | ||
| 'woocommerce-composite-products' => $this->is_woocommerce_composite_products_active(), | ||
| 'woocommerce-shipping-per-product' => $this->is_woocommerce_shipping_per_product_active(), | ||
| 'woocommerce-deposits' => $this->is_woocommerce_deposits_active(), |
There was a problem hiding this comment.
idea: turn these strings into public consts
Reasons:
- Allows auto-completion in the IDE
- Avoids typos
- Gives us a single place with the full detection-catalog
There was a problem hiding this comment.
Good point, thanks! Done.
|
|
||
| $addons = \WC_Product_Addons_Helper::get_product_addons( $product->get_id(), false, false, false ); | ||
|
|
||
| return array() !== $addons; |
There was a problem hiding this comment.
nit: Rewrite as count( $addons ) > 0 or ! empty( $addons )
Reason: makes the decision a bit easier to understand
| $this->logger->shouldReceive( 'warning' ) | ||
| ->once() | ||
| ->with( | ||
| Mockery::on( | ||
| static function ( string $message ) use ( $plugin ): bool { | ||
| return false !== strpos( $message, $plugin ) && false !== strpos( $message, 'does not exist' ); | ||
| } | ||
| ) | ||
| ); |
There was a problem hiding this comment.
suggestion: Remove this logger expectation
Reason: Same as above, It's not helping with plugin stability and does not prevent a bug. If devs need additional or different logs, it should not require unit test changes.
The ->assertFalse() check below is exactly what this test should verify
There was a problem hiding this comment.
I explained above why I'd rather keep this. But I'm open to a discussion if you really believe it shouldn't be here.
| /** | ||
| * Class RuntimeException | ||
| */ | ||
| class RuntimeException extends \RuntimeException { |
There was a problem hiding this comment.
change: Find a unique name for this
Reason: This class shadows the native RuntimeException which can (and likely will) cause confusion. In the worst case, someone might think "we do not need this class" and delete it, breaking the ProductCustomizationDetector try-catch
(Claude suggests PluginApiChangedException or DetectionMethodNotFoundException)
There was a problem hiding this comment.
Good point, renamed.
Test using WordPress PlaygroundThe changes in this pull request can be previewed and tested using a WordPress Playground instance. 🔗 Test this pull request with WordPress Playground What's included:
Login credentials:
Plugin Details:
🤖 Auto-generated for commit 98123db • Last updated: 2026-07-23T11:34:00.549Z |
| * @param PluginDetectorInterface $plugin_detector The plugin presence detector. | ||
| * @param LoggerInterface $logger The logger. | ||
| * @param PluginDetector $plugin_detector The plugin presence detector. | ||
| * @param LoggerInterface $logger The logger. |
There was a problem hiding this comment.
I would also suggest to remove these comments as they do not convey any info
There was a problem hiding this comment.
Agree, they are useless. Removed.
Description
Provides two services for:
Steps to Test
wp ppcp-compat-products scanPlugins list
Products to import
wc-product-export-22-7-2026-1784711315948.csv
Plugin for testing
ppcp-compat-products.zip
Documentation
So far, only internal documentation is needed. This PR doesn't produce any visible changes; it only registers services which aren't used yet.
Notes