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

Add test to verify database parameter is propagated to Predis #208

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

pauortegathoughtworks
Copy link

@pauortegathoughtworks pauortegathoughtworks commented Jun 8, 2023

PHP SDK

What did you accomplish?

Since the Split Synchronizer documentation recommends using a dedicated Redis database for splits users might need to specify the database they're using when initializing the SDK instance.

$parameters = array(
    'scheme' => 'redis',
    'host' => REDIS_HOST,
    'port' => REDIS_PORT,
    'timeout' => 881,
    'database' => $database
);
$options = array('prefix' => TEST_PREFIX);

$sdkConfig = array(
    'log' => array('adapter' => 'stdout'),
    'cache' => array('adapter' => 'predis', 'parameters' => $parameters, 'options' => $options)
);

//Initializing the SDK instance.
$splitFactory = \SplitIO\Sdk::factory('asdqwe123456', $sdkConfig);
$splitSdk = $splitFactory->client();

While this functionality already exists it is:

a) not documented in Split.io PHP SDK page
b) as far as I can tell, not tested

This PR is aimed at fixing point b), and raising the question on how to fix point a).

How do we test the changes introduced in this PR?

The change is the test: we added a testDatabaseParameterIsPropagatedToPredisClient in SdkClientTest to verify the database parameter is propagated to the Predis client.

Extra Notes

How can we address point a), that is, how can we make it obvious in Split.io PHP SDK page that database is an accepted parameter?

@pauortegathoughtworks pauortegathoughtworks changed the base branch from master to develop June 8, 2023 14:44
@pauortegathoughtworks pauortegathoughtworks marked this pull request as ready for review June 12, 2023 08:46
@pauortegathoughtworks pauortegathoughtworks requested a review from a team as a code owner November 8, 2023 08:50
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.

1 participant