Config v2 branch #1
Conversation
| * | ||
| * @var [Attribute] | ||
| */ | ||
| private $_attributes; |
There was a problem hiding this comment.
no need to prefix variable name with underscore for private variables. It was old practice. Also revise the same for all new variables you have added.
| /** | ||
| * @var string environmentKey of the config. | ||
| */ | ||
| private $environment_key; |
There was a problem hiding this comment.
PHP uses camel casing to name variables. Also revise others too
| $this->_attributes = $config['attributes'] ?: []; | ||
| $this->_audiences = $config['audiences'] ?: []; | ||
| $this->_events = $config['events'] ?: []; | ||
| $this->_typed_audiences = isset($config['typedAudiences']) ? $config['typedAudiences']: []; |
There was a problem hiding this comment.
local variables for same are declared below as well. Get rid of local variables and use these new variables in below code.
| @@ -93,6 +93,18 @@ class DatafileProjectConfig implements ProjectConfigInterface | |||
| */ | |||
| private $datafile; | |||
|
|
|||
| /** | |||
There was a problem hiding this comment.
update header of this file and all others. 2021
| /** | ||
| * @var string sdkKey of the config. | ||
| */ | ||
| private $sdk_key; |
| /** | ||
| * @var string Contents of datafile. | ||
| */ | ||
| private $datafile; | ||
|
|
||
|
|
||
| public function __construct($revision, array $experimentsMap, array $featuresMap, $datafile = null) | ||
| public function __construct($revision, array $experimentsMap, array $featuresMap, $datafile = null, $environment_key='', $sdk_key='', array $attributes=[], array $audiences=[], array $events=[]) |
| $id = $attr["id"]; | ||
| $key = $attr["key"]; | ||
| $optly_attr = new OptimizelyAttribute( | ||
| $id, |
There was a problem hiding this comment.
directly use $attr["id"]; here instead of assigning both id and key to local variables first
| */ | ||
| protected function getConfigAttributes() | ||
| { | ||
| $attribut_arr = []; |
| @@ -207,6 +318,85 @@ protected function getVariationsMap(Experiment $experiment) | |||
| return $variationsMap; | |||
| } | |||
|
|
|||
| /** | |||
| * Generates array of delivery rules for optimizelyFeature. | |||
| * | ||
| * @return array of optimizelyExperiments as delivery rules . | ||
| */ | ||
| protected function getExperimentAudiences(array $audience_cond) |
| foreach ($normalAudiences as $naudience) { | ||
| $id = $naudience['id']; | ||
| if (in_array($id, $uniqueIds) == false) { | ||
| array_push($audiencesArray, $naudience); |
There was a problem hiding this comment.
instead of false put a ! before in_array
| $resAudiences = ''; | ||
| $audienceConditions = array('and', 'or', 'not'); | ||
| if ($audienceCond != null){ | ||
| $cond = ''; |
There was a problem hiding this comment.
Return If audience cond is null. This way you can reduce 1 level nesting
| @@ -223,11 +402,21 @@ protected function getExperimentsMaps() | |||
| foreach ($this->experiments as $exp) { | |||
| $expId = $exp->getId(); | |||
| $expKey = $exp->getKey(); | |||
| $audiences = ''; | |||
| if (is_null($exp->getAudienceConditions())) { | |||
There was a problem hiding this comment.
instead of is_null use === null to avoid an extra function call
There was a problem hiding this comment.
No need to put this condition audiences is already empty str. Just put the else logic in an if block
| $expId = $exp->getId(); | ||
| $expKey = $exp->getKey(); | ||
| $audiences = ''; | ||
| if (is_null($exp->getAudienceConditions())) { |
There was a problem hiding this comment.
Same as above. No need for this condition. You are setting the same value
| $experimentRules = []; | ||
| $deliveryRules = []; | ||
| $rollout_id = $feature->getRolloutId(); | ||
| if (is_null($rollout_id)) |
| * @return string Experiment audiences. | ||
| */ | ||
| public function getExperimentAudiences() | ||
| { |
| /** | ||
| * list of Audiences that will be parsed from the datafile | ||
| * | ||
| * @var [Audiences] |
There was a problem hiding this comment.
| * @var [Audiences] | |
| * @var [Audience] |
| @@ -51,6 +51,37 @@ public function getBotFiltering(); | |||
| */ | |||
| public function getRevision(); | |||
|
|
|||
| /** | |||
| * @return string String represnting envkey of the datafile. | |||
There was a problem hiding this comment.
| * @return string String represnting envkey of the datafile. | |
| * @return string String representing environment key of the datafile. |
| @@ -1,6 +1,6 @@ | |||
| <?php | |||
| /** | |||
| * Copyright 2020, Optimizely Inc and Contributors | |||
| * Copyright 2021, Optimizely Inc and Contributors | |||
There was a problem hiding this comment.
| * Copyright 2021, Optimizely Inc and Contributors | |
| * Copyright 2020-2021, Optimizely Inc and Contributors |
| /** | ||
| * array of attributes as OptimizelyAttribute. | ||
| * | ||
| * @var <String, OptimizelyAttribute> associative array |
There was a problem hiding this comment.
Please fix this comment, this is not supposed to be a map.
|
|
||
|
|
||
| /** | ||
| * Map of rollout Experiments Keys to OptimizelyExperiments. |
| @@ -66,6 +84,22 @@ public function getKey() | |||
| return $this->key; | |||
| } | |||
|
|
|||
| /** | |||
| * @return array Map of Experiment Keys to OptimizelyExperiments. | |||
| } | ||
|
|
||
| /** | ||
| * @return array Map of Rollout Experiments Keys to OptimizelyExperiments. |
There was a problem hiding this comment.
This is not a map, it's an array. Please verify the comment from other SDKs.
| @@ -28,17 +28,24 @@ class OptimizelyExperiment implements \JsonSerializable | |||
| */ | |||
There was a problem hiding this comment.
Please update header file with 2020-2021
| @@ -1,6 +1,6 @@ | |||
| <?php | |||
| /** | |||
| * Copyright 2020, Optimizely | |||
| * Copyright 2021, Optimizely | |||
There was a problem hiding this comment.
| * Copyright 2021, Optimizely | |
| * Copyright 2020-2021, Optimizely |
| @@ -51,6 +51,37 @@ public function getBotFiltering(); | |||
| */ | |||
| public function getRevision(); | |||
|
|
|||
| /** | |||
| * @return string String represnting representing environment key of the datafile. | |||
| $attributArray = []; | ||
| $attributes = $this->projectConfig->getAttributes(); | ||
| foreach ($attributes as $attr) { | ||
| $optly_attr = new OptimizelyAttribute( |
| $attr['id'], | ||
| $attr['key'] | ||
| ); | ||
| array_push($attributArray, $optly_attr); |
| $eventsArray = []; | ||
| $events = $this->projectConfig->getEvents(); | ||
| foreach ($events as $event) { | ||
| $optly_event = new OptimizelyEvent( |
| $audiencesArray = $typedAudiences; | ||
| foreach ($audiencesArray as $typedAudience) { | ||
| $id = $typedAudience['id']; | ||
| array_push($uniqueIds, $id); |
There was a problem hiding this comment.
directly push $typedAudience['id'];. No need to assign it to a variable here
| $id = $audience['id']; | ||
| if ($id != '$opt_dummy_audience') { | ||
| $optly_audience = new OptimizelyAudience( | ||
| $audience['id'], |
There was a problem hiding this comment.
use $id here. Or remove $id assignment above.
| @@ -210,8 +251,8 @@ class DatafileProjectConfig implements ProjectConfigInterface | |||
| /** | |||
| * DatafileProjectConfig constructor to load and set project configuration data. | |||
| * | |||
| * @param $datafile string JSON string representing the project. | |||
| * @param $logger LoggerInterface | |||
| * @param $datafile string JSON string representing the project. | |||
| $this->attributes = $config['attributes'] ?: []; | ||
| $this->audiences = $config['audiences'] ?: []; | ||
| $this->events = $config['events'] ?: []; | ||
| $this->typedAudiences = isset($config['typedAudiences']) ? $config['typedAudiences'] : []; |
There was a problem hiding this comment.
why this can't be $config['typedAudiences'] ?: []
There was a problem hiding this comment.
i just followed the existing way to get typedAudiences ealier it was being used as a variable but i made it a class attribute.
| $this->events = $config['events'] ?: []; | ||
| $this->typedAudiences = isset($config['typedAudiences']) ? $config['typedAudiences'] : []; | ||
| $this->_anonymizeIP = isset($config['anonymizeIP']) ? $config['anonymizeIP'] : false; | ||
| $this->_botFiltering = isset($config['botFiltering']) ? $config['botFiltering'] : null; |
There was a problem hiding this comment.
what's the default value, why specifying explicit null.
There was a problem hiding this comment.
default value for botFiltering was not mentioned anywhere i did not change this line it is as it was.
| $this->_revision = $config['revision']; | ||
| $this->_sendFlagDecisions = isset($config['sendFlagDecisions']) ? $config['sendFlagDecisions'] : false; | ||
|
|
||
| $groups = $config['groups'] ?: []; | ||
| $experiments = $config['experiments'] ?: []; | ||
| $events = $config['events'] ?: []; |
There was a problem hiding this comment.
they were being used as variables but i need to get them as attributes so delete the variables and using attributes in further functions
| * @param bool $skipJsonValidation boolean representing whether JSON schema validation needs to be performed. | ||
| * @param LoggerInterface $logger Logger instance | ||
| * @param ErrorHandlerInterface $errorHandler ErrorHandler instance. | ||
| * @param string $datafile JSON string representing the Optimizely project. |
| return array_filter(array_values($this->_experimentKeyMap), function ($experiment) use ($rolloutExperimentIds) { | ||
| return !in_array($experiment->getId(), $rolloutExperimentIds); | ||
| }); | ||
| return array_filter( |
There was a problem hiding this comment.
don't change just for the sake of change. please.
| /** | ||
| * @return array List of attributes parsed from the datafile | ||
| */ | ||
| public function getAttributes(); |
There was a problem hiding this comment.
following are not a part of ProjectConfigInterface
There was a problem hiding this comment.
attributes, audiences, events, typedAudiences.
check in other sdks, how retrieving arrays.
| $rollouts = isset($config['rollouts']) ? $config['rollouts'] : []; | ||
| $featureFlags = isset($config['featureFlags']) ? $config['featureFlags']: []; | ||
| $featureFlags = isset($config['featureFlags']) ? $config['featureFlags'] : []; |
There was a problem hiding this comment.
avoid nit changes in parts of code which are not changed. Change it back to what it was.
There was a problem hiding this comment.
actually i started my work before the pr of duplicate exp key issue was merged with master so i just update my branch with the latest master branch to use that instead of doing that task again
| @@ -627,14 +721,15 @@ public function getAttribute($attributeKey) | |||
|
|
|||
| /** | |||
| * @param $experimentKey string Key for experiment. | |||
| * @param $variationKey string Key for variation. | |||
| * @param $variationKey string Key for variation. | |||
There was a problem hiding this comment.
actually i started my work before the pr of duplicate exp key issue was merged with master so i just update my branch with the latest master branch to use that instead of doing that task again
| * | ||
| * @return Variation Entity corresponding to the provided experiment key and variation key. | ||
| * Dummy entity is returned if key or ID is invalid. | ||
| */ | ||
| public function getVariationFromKey($experimentKey, $variationKey) | ||
| { | ||
| if (isset($this->_variationKeyMap[$experimentKey]) | ||
| if ( |
There was a problem hiding this comment.
change all the extra nit changes you have done back to as it was. make another PR if you want to fix these nits.
| private $id; | ||
|
|
||
| /** | ||
| * @var string name representing audience. |
There was a problem hiding this comment.
| * @var string name representing audience. | |
| * @var string representing audience name. |
| class OptimizelyAudience implements \JsonSerializable | ||
| { | ||
| /** | ||
| * @var string id representing audience. |
There was a problem hiding this comment.
| * @var string id representing audience. | |
| * @var string representing audience id. |
| @@ -207,6 +307,77 @@ protected function getVariationsMap(Experiment $experiment) | |||
| return $variationsMap; | |||
| } | |||
|
|
|||
| /** | |||
| * Generates string of audience conditions array mapped to audience names. | |||
| * | |||
There was a problem hiding this comment.
add some examples in comments as well see csharp link
| protected function getSerializedAudiences(array $audienceConditions) | ||
| { | ||
| $finalAudiences = ''; | ||
| $ConditionsArray = array('and', 'or', 'not'); |
There was a problem hiding this comment.
instead of initializing it over here can we move it outside to function to avoid initializing it over and over as you are also using recursion.
There was a problem hiding this comment.
i have to initialize it in constructor which makes it more annoying.
| * | ||
| * @return string of experiment audience conditions. | ||
| */ | ||
| protected function getSerializedAudiences(array $audienceConditions) |
There was a problem hiding this comment.
add comments in the function and explain the logic see Csharp link
| if ($audience == null) { | ||
| $finalAudiences = $finalAudiences . $cond . ' ' . '"' . $itemStr . '"'; | ||
| } else { | ||
| $name = $audience->getName(); |
There was a problem hiding this comment.
can we refactor this logic and take name before checking audience.
something like this
$name = $audience == null ? $itemStr : $audience->getName();
we can then avoid condition of audience == null and use
$finalAudiences = $finalAudiences . $cond . ' ' . '"' . $name. '"';
| $this->optConfigService, | ||
| array($audienceConditions[$testNo]) | ||
| ); | ||
| //fwrite(STDERR, print_r(gettype($response), true)); |
Summary
The "why", or other context.
Test plan
Issues