Issue / 10169 Plugin Detection Logic #10209

10upsimon · 2025-02-12T12:40:46Z

Summary

Addresses issue:

Add Plugin Detection Logic to Ads Module Backend Infrastructure #10169

Relevant technical choices

PR Author Checklist

My code is tested and passes existing unit tests.
My code has an appropriate set of unit tests which all pass.
My code is backward-compatible with WordPress 5.2 and PHP 7.4.
My code follows the WordPress coding standards.
My code has proper inline documentation.
I have added a QA Brief on the issue linked above.
I have signed the Contributor License Agreement (see https://cla.developers.google.com/).

Do not alter or remove anything below. The following sections will be managed by moderators only.

Code Reviewer Checklist

Run the code.
Ensure the acceptance criteria are satisfied.
Reassess the implementation with the IB.
Ensure no unrelated changes are included.
Ensure CI checks pass.
Check Storybook where applicable.
Ensure there is a QA Brief.
Ensure there are no unexpected significant changes to file sizes.

Merge Reviewer Checklist

Ensure the PR has the correct target branch.
Double-check that the PR is okay to be merged.
Ensure the corresponding issue has a ZenHub release assigned.
Add a changelog message to the issue.

…us util.

… helper util for WooCommerce plugin detection.

github-actions · 2025-02-12T12:47:44Z

Build files for b3f3ae2 are ready:

Download development build
Download production build

10upsimon · 2025-02-12T13:41:18Z

Note to reviewer

Please see this comment against the issue as to why the implementation is slightly different to the IB, thank you.

zutigrm

Thanks @10upsimon looks good overall, I left you a few comments

zutigrm · 2025-02-12T14:32:30Z

includes/Core/Assets/Assets.php

@@ -760,6 +761,54 @@ private function get_inline_base_data() {
 			'isMultisite'       => is_multisite(),
 		);

+		if ( Feature_Flags::enabled( 'adsPax' ) ) {
+			$inline_data['plugins'] = array(


Not sure we need this additional nesting, it only complicates already nested structure 🤔 . It does related to plugins, although this goes into the core site namespace, and plugins are technically within the site scope

zutigrm · 2025-02-12T14:33:28Z

includes/Core/Assets/Assets.php

@@ -18,6 +18,7 @@
 use Google\Site_Kit\Core\Util\URL;
 use WP_Dependencies;
 use WP_Post_Type;
+use Google\Site_Kit\Core\Util\Plugin_Status;


Let's group this import together with other imports from Site_Kit namespace

zutigrm · 2025-02-12T14:36:47Z

includes/Core/Assets/Assets.php

+			// Get the status of the WooCommerce plugin.
+			$woocommerce_plugin_status = Plugin_Status::get_plugin_status( 'woocommerce/woocommerce.php', 'https://woocommerce.com/' );
+
+			// Get the status of the Google for WooCommerce plugin.
+			$google_for_woocommerce_plugin_status = Plugin_Status::get_plugin_status( 'google-listings-and-ads/google-listings-and-ads.php', 'https://wordpress.org/plugins/google-listings-and-ads' );
+
+			switch ( $woocommerce_plugin_status ) {
+				case Plugin_Status::PLUGIN_STATUS_ACTIVE:
+					$inline_data['plugins']['woocommerce']['installed'] = true;
+					$inline_data['plugins']['woocommerce']['active']    = true;
+					break;
+				case Plugin_Status::PLUGIN_STATUS_INSTALLED:
+					$inline_data['plugins']['woocommerce']['installed'] = true;
+					$inline_data['plugins']['woocommerce']['active']    = false;
+					break;
+			}
+
+			switch ( $google_for_woocommerce_plugin_status ) {
+				case Plugin_Status::PLUGIN_STATUS_ACTIVE:
+					$inline_data['plugins']['google-listings-and-ads']['installed'] = true;
+					$inline_data['plugins']['google-listings-and-ads']['active']    = true;
+


Perhaps we can extract woo and GfW logic of computing the inline data, and return of the inline data itself into a separate module, to keep both of the methods more compact

zutigrm · 2025-02-12T14:41:13Z

includes/Core/Consent_Mode/REST_Consent_Mode_Controller.php

-								} else {
-									foreach ( $plugins as $plugin_file => $installed_plugin ) {
-										if ( $installed_plugin['PluginURI'] === $plugin_uri ) {
-											$plugin    = $plugin_file;


Are we sure it is safe to remove this re-assignment of $plugin? We do define it at the top of the function based on the known path, which should be enough, but there might be instance it was not enough that we had to match it by URI and re-assign it, might be worthwhile confirming this @nfmohit who did this initial implementation

but there might be instance it was not enough that we had to match it by URI and re-assign it

@zutigrm we do this further second matching on the Plugin_Status class now, so it should be okay? cc @nfmohit

zutigrm · 2025-02-12T15:00:24Z

includes/Modules/Sign_In_With_Google.php

@@ -713,7 +705,7 @@ protected function inline_module_data( $modules_data ) {
 			$inline_data['existingClientID'] = $existing_client_id;
 		}

-		$is_woocommerce_active            = $this->woocommerce->is_active();
+		$is_woocommerce_active            = Plugin_Status::PLUGIN_STATUS_ACTIVE === Plugin_Status::get_plugin_status( 'woocommerce/woocommerce.php', 'https://woocommerce.com/' );


We use this check in more then one place, it will be more efficient to wrap it within a separate method, like is_woocommerce_active that will handle this, and we can also store the result within the class property to avoid re-fetching the plugins and re-doing the same loop. We could call this method during the constructor or register method for example

zutigrm · 2025-02-12T15:39:53Z

tests/phpunit/integration/Core/Assets/AssetsTest.php

+	protected function enable_feature( $feature ) {
+		$enable_callback = function ( $enabled, $feature_name ) use ( $feature ) {
+			if ( $feature_name === $feature ) {
+				return true;
+			}
+			return $enabled;
+		};
+
+		add_filter( 'googlesitekit_is_feature_enabled', $enable_callback, 10, 2 );
+
+		return function () use ( $enable_callback ) {
+			remove_filter( 'googlesitekit_is_feature_enabled', $enable_callback, 10 );
+		};
+	}


This is already defined in the main TestCase class

site-kit-wp/tests/phpunit/includes/TestCase.php

Lines 89 to 102 in 91fb322

protected function enable_feature( $feature ) {

$enable_callback = function ( $enabled, $feature_name ) use ( $feature ) {

if ( $feature_name === $feature ) {

return true;

}

return $enabled;

};

add_filter( 'googlesitekit_is_feature_enabled', $enable_callback, 10, 2 );

return function () use ( $enable_callback ) {

remove_filter( 'googlesitekit_is_feature_enabled', $enable_callback, 10 );

};

}

which this test class extends, so this can be removed as there is no reason to override it

zutigrm · 2025-02-12T15:46:53Z

includes/Core/Assets/Assets.php

+					'installed' => false,
+					'active'    => false,
+				),
+				'google-listings-and-ads' => array(


Sorry, one more thing, I know we planned to use this slug in the IB, but let's rename it to googleForWooCommerce, to be more clear what plugin is this about since that's how it is called now, and to adhere to general approach of not having kebab-case properties in the inline data, or data stores in general (with modules being exception since we have to adhere to the module slug everywhere)

10upsimon added 12 commits January 28, 2025 10:28

Merge branch 'develop' of github.com:google/site-kit-wp into develop.

6f931c1

Merge branch 'develop' of github.com:google/site-kit-wp into develop.

5914f7c

Merge branch 'develop' of github.com:google/site-kit-wp into develop.

6913e87

Merge branch 'develop' of github.com:google/site-kit-wp into develop.

41bd763

Merge branch 'develop' of github.com:google/site-kit-wp into develop.

1369812

Add plugins output to base data.

abbce03

Update AssetsTest suite to cater for new base data.

510e4ba

Update WooCommerce provider to user new Plugin_Status util.

70ef21e

Update WooCommerceTest suite following integration of new Plugin_Stat…

4d35d36

…us util.

Update REST_Consent_Mode_Controller to use new Plugin_Status util.

dc74e6f

Upate Sign_In_With_Google module entry point to use new Plugin_Status…

5511753

… helper util for WooCommerce plugin detection.

Create new Plugin_Status util class.

ba4e30c

10upsimon added 2 commits February 12, 2025 15:01

Add basic test coverage for Plugin_Status class.

235742a

Update activate_plugin helper method in Plugin_StatusTest.

b3f3ae2

zutigrm requested changes Feb 12, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue / 10169 Plugin Detection Logic #10209

Issue / 10169 Plugin Detection Logic #10209

10upsimon commented Feb 12, 2025 •

edited

Loading

github-actions bot commented Feb 12, 2025 •

edited

Loading

10upsimon commented Feb 12, 2025 •

edited

Loading

zutigrm left a comment

zutigrm Feb 12, 2025

zutigrm Feb 12, 2025

zutigrm Feb 12, 2025

zutigrm Feb 12, 2025

10upsimon Feb 12, 2025

zutigrm Feb 12, 2025

zutigrm Feb 12, 2025

zutigrm Feb 12, 2025

	protected function enable_feature( $feature ) {
	$enable_callback = function ( $enabled, $feature_name ) use ( $feature ) {
	if ( $feature_name === $feature ) {
	return true;
	}
	return $enabled;
	};

	add_filter( 'googlesitekit_is_feature_enabled', $enable_callback, 10, 2 );

	return function () use ( $enable_callback ) {
	remove_filter( 'googlesitekit_is_feature_enabled', $enable_callback, 10 );
	};
	}

Issue / 10169 Plugin Detection Logic #10209

Are you sure you want to change the base?

Issue / 10169 Plugin Detection Logic #10209

Conversation

10upsimon commented Feb 12, 2025 • edited Loading

Summary

Relevant technical choices

PR Author Checklist

Code Reviewer Checklist

Merge Reviewer Checklist

github-actions bot commented Feb 12, 2025 • edited Loading

10upsimon commented Feb 12, 2025 • edited Loading

Note to reviewer

zutigrm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

10upsimon commented Feb 12, 2025 •

edited

Loading

github-actions bot commented Feb 12, 2025 •

edited

Loading

10upsimon commented Feb 12, 2025 •

edited

Loading