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

Ensure you can properly remove Providers #456

Merged
merged 6 commits into from
May 18, 2023
Merged

Conversation

dkotter
Copy link
Collaborator

@dkotter dkotter commented May 12, 2023

Description of the Change

There's an existing filter, {$this->menu_slug}_providers, that allows you to filter the available Providers for each Service. This filter has been around for 4 years but in testing it out, it doesn't work properly.

The problem is we have a number of places where we reference our Provider Service with code like $this->provider_classes[0]. There are two issues with this:

  1. We assume the Provider Service we want is always going to be the first within the array, which may not be true now that we've started to add multiple Services in each Provider
  2. It assumes this is always an array that has at least one value. This is the issue that is preventing full usage of the mentioned filter

This PR fixes the second point in its entirety by making sure we always have an array of at least one Provider Service before we use that variable. This allows you to remove all Services from a particular Provider and not get any PHP errors.

It mostly fixes the first point as well by adding a new find_provider_class helper function to get the Provider Service based on the Provider name, instead of position within the array. I think there is still one place that references the Provider Service by array position but it seemed fine to leave for now.

If no Providers are registered within a Service, the Service still shows up in our settings but no inputs show and instead we show an error message:

Service removed

Options also won't show in the new onboarding:

Only have a single Service registered

This helps with #404 but doesn't completely close it

How to test the Change

  1. Either within your theme's functions.php or within a custom plugin, utilize the {$this->menu_slug}_providers filter to remove one or more Services. Here's the code I've been using:
add_action(
	'init',
	function() {
		$services = [
			'image_processing',
			'language_processing',
			'personalizer',
		];

		foreach ( $services as $service ) {
			add_filter(
				"{$service}_providers",
				function( array $providers = [] ) use ( $service ) {
					if ( 'image_processing' === $service ) {
						/**
						 * Default providers:
						 * 'Classifai\Providers\Azure\ComputerVision'
						 * 'Classifai\Providers\OpenAI\DallE'
						 */
						return [ 'Classifai\Providers\OpenAI\DallE' ];
					}

					if ( 'language_processing' === $service ) {
						/**
						 * Default providers:
						 * 'Classifai\Providers\Watson\NLU'
						 * 'Classifai\Providers\OpenAI\ChatGPT'
						 */
						return [];
					}

					if ( 'personalizer' === $service ) {
						/**
						 * Default providers:
						 * Classifai\Providers\Azure\Personalizer
						 */
						return [];
					}
				}
			);
		}
	}
);
  1. Go to the ClassifAI settings and ensure the Services you've removed don't show any options but the ones you've left do show proper options
  2. Ensure whichever Services you've left still work properly
  3. Remove the above filter so all Services are back and ensure settings for all of them show and that all features work as expected
  4. Ensure no PHP warnings/errors are logged at any step

Changelog Entry

Fixed - Ensure the {$this->menu_slug}_providers filter works as expected when used to remove Providers

Credits

Props @dkotter

Checklist:

  • I agree to follow this project's Code of Conduct.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests pass.

@dkotter dkotter added this to the 2.2.0 milestone May 12, 2023
@dkotter dkotter requested a review from a team as a code owner May 12, 2023 03:06
@dkotter dkotter self-assigned this May 12, 2023
@dkotter dkotter requested a review from jeffpaul as a code owner May 12, 2023 03:06
iamdharmesh
iamdharmesh previously approved these changes May 18, 2023
Copy link
Member

@iamdharmesh iamdharmesh left a comment

Choose a reason for hiding this comment

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

@dkotter Thanks for working on this and making the classifAI more extendible. LGTM 🚀

includes/Classifai/Services/Service.php Show resolved Hide resolved
@dkotter dkotter merged commit 3a6dda5 into develop May 18, 2023
12 of 13 checks passed
@dkotter dkotter deleted the fix/disable-providers branch May 18, 2023 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants