Please vendor prefix Google-related composer dependencies

[ecaron]

Describe the bug:

If other plugins use Google SDKs, there's a namespace conflict that happens where other plugins using (for example) apiclient and are expecting GuzzleHttpClientInterface but receive AutomatticWooCommerceGoogleListingsAndAdsVendorGuzzleHttpClient.

A detailed example of the error condition: Fatal error: Uncaught TypeError: GoogleClient::setHttpClient(): Argument #1 ($http) must be of type GuzzleHttpClientInterface, AutomatticWooCommerceGoogleListingsAndAdsVendorGuzzleHttpClient given in /code/wp-content/plugins/customer-plugin/vendor/google/apiclient/src/Client.php:1152 Stack trace: #0 [internal function]: GoogleClient->setHttpClient(Object(AutomatticWooCommerceGoogleListingsAndAdsVendorGuzzleHttpClient)) #1 /code/wp-content/plugins/google-listings-and-ads/vendor/league/container/src/Definition/Definition.php(273): call_user_func_array(Array, Array) #2 /code/wp-content/plugins/google-listings-and-ads/vendor/league/container/src/Definition/Definition.php(216): AutomatticWooCommerceGoogleListingsAndAdsVendorLeagueContainerDefinitionDefinition->invokeMethods(Object(GoogleClient))

Tickets #314 and #1643 might be impacted by this.

Steps to reproduce:

1. Install another plugin using stock apiclient
2. Install Google Listings and Ads

Expected behavior:

Both plugins work.

Actual behavior:

Site crashes because of type errors due to inconsistent SDK behavior between the installed plugins.

Additional details:

Other plugins that have similar "add vendor prefixing" tickets:

• WooCommerce
• Yoast

[mikkamp]

Thanks for the report @ecaron

Is it just the apiclient library that is being used by the plugin? Or are there other Google packages that are conflicting as well? We do already add a vendor prefix to a select list of composer packages, however apiclient is not on that list. So it would be helpful to know a bit more about the plugin to decide which additional packages to include.

Adding apiclient to the list sounds like a reasonable request, but I imagine that means we would need to do the same with apiclient-services.

The reason why your specific use case breaks the site is because the plugin using the apiclient package is called customer-plugin, since plugins are loaded in alphabetical order that means it's loaded before google-listings-and-ads. For arguments sake we could also reverse this request, if the customer-plugin would add a vendor prefix it would also resolve the conflict. Which is why a bit more context would be helpful so we can prioritize this request accordingly.

[ecaron]

@mikkamp Our plugin also requires https://packagist.org/packages/google/cloud-recaptcha-enterprise (which requires google/gax, so that'll be another clash). In addition to apiclient, of course.

If the plugin load order was reversed, wouldn't google-listings-and-ads break due to google-listings-and-ads's GuzzleHttp expecting AutomatticWooCommerceGoogleListingsAndAdsVendorGuzzleHttpClient but getting GuzzleHttpClientInterface?

[mikkamp]

Thanks for clarifying the additional packages.

Reversing the plugin load order in itself is not enough to reduce the conflicts, because like you mentioned it will just be a conflict the other way around. I meant to imply the following two scenarios:

1. google-listings-and-ads adds vendor prefix for remaining packages
   • google-listings-and-ads does not conflict with customer-plugin or other plugins
   • customer-plugin will still conflict with any other plugin that uses the same packages
2. customer-plugin adds vendor prefix for composer packages
   • customer-plugin doesn't conflict with google-listings-and-ads or other plugins
   • google-listings-and-ads could still conflict with other plugins

[ecaron]

@mikkamp That makes sense. We'd been trying to make sure any of our plugins that used composer & weren't uniquely namespaced played well with each other, but this will probably be a situation that forces us to re-consider that position (maybe implementing something like https://github.com/BrianHenryIE/strauss).

For google-listings-and-ads, would you consider an all-on-none policy, where any package that's modified becomes a part of an isolated namespace? This way you could still have some packages that don't have the burden of getting tweaked-per-build, but you'd also protect other code that happens to rely on the same package can expect default behaviors?

[mikkamp]

would you consider an all-on-none policy, where any package that's modified becomes a part of an isolated namespace?

We'd definitely take it into consideration. But we'll have to revisit this again when we decide what path to take when resolving the remaining conflicts.

[ecaron]

FWIW, we're running a version that removes all the namespacing - https://github.com/woocommerce/google-listings-and-ads/compare/develop...CaribouCoffee:google-listings-and-ads:noDeps - we don't intend it to become a PR, but it let us resume using GLaA and our other plugin, so the urgency has reduced.

[mikkamp]

Additional note in regards to PR #1859

It only partially resolves this issue as it additionally prefixes the google/apiclient and google/apiclient-services packages.

The structure is now in place to prefix other Google packages as well, but we'll want to take it one step at at time since prefixing them all could have bigger implications. The next step would be to prefix packages used for Google Ads (including google-gax).

[BrianHenryIE]

Did you try BrianHenryIE/strauss ? It would do most of what you described in that PR and the rest would be a valuable contribution to Strauss.

[mikkamp]

Did you try BrianHenryIE/strauss ? It would do most of what you described in that PR and the rest would be a valuable contribution to Strauss.

Thanks for the suggestion, I have not tried Strauss yet. Based on an initial glance it looks like it covers some of the functionality which we were missing in Mozart so it definitely looks like it has great potential. I'll have to experiment with it a bit more to see if we can reuse it now or whether to leave it on the list for when we get round to the next iteration.