feat: datafile accessor #211
Conversation
|
@ozayr-zaviar can you take care of signing our CLA mentioned in the CONTRIBUTING document? |
| @@ -159,4 +159,11 @@ public function getFeatureVariableFromKey($featureFlagKey, $variableKey); | |||
| * @return boolean A boolean value that indicates if the experiment is a feature test. | |||
| */ | |||
| public function isFeatureExperiment($experimentId); | |||
|
|
|||
| /** | |||
| * Determines if given experiment is a feature test. | |||
| @@ -63,6 +68,7 @@ public function __construct(ProjectConfigInterface $projectConfig) | |||
| $this->experiments = $projectConfig->getAllExperiments(); | |||
| $this->featureFlags = $projectConfig->getFeatureFlags(); | |||
| $this->revision = $projectConfig->getRevision(); | |||
| $this->datafile = $$projectConfig->toDatafile(); | |||
There was a problem hiding this comment.
You haven't passed datafile to new OptimizelyConfig below
| private $datafile; | ||
|
|
||
|
|
||
| public function __construct($revision, array $experimentsMap, array $featuresMap, $datafile = null) |
There was a problem hiding this comment.
do not provide a default null for datafile.
| @@ -14,709 +14,464 @@ | |||
| * See the License for the specific language governing permissions and | |||
| * limitations under the License. | |||
| */ | |||
|
|
|||
| namespace Optimizely\Tests; | |||
There was a problem hiding this comment.
Please fix this file. This should only show diff of the new unit test you have added.
| public function testGetDatafile() | ||
| { | ||
| $expectedDatafile = DATAFILE_FOR_OPTIMIZELY_CONFIG; | ||
| $actualDatafile = $this->optConfigService->getDatafile(); |
There was a problem hiding this comment.
getDatafile() has been defined for OptimizelyConfig, not OptimizelyConfigService.
|
@msohailhussain Testapp PR should be merged to get FSC passed https://github.com/optimizely/php-testapp/pull/65 |
| /** | ||
| * @var string datafile. | ||
| */ | ||
| private $datafile; |
There was a problem hiding this comment.
shouldn't it be $_datafile?
| /** | ||
| * @var string Contents of datafile. | ||
| */ | ||
| private $datafile; |
There was a problem hiding this comment.
| private $datafile; | ||
|
|
||
|
|
||
| public function __construct($revision, array $experimentsMap, array $featuresMap, $datafile) |
There was a problem hiding this comment.
won't it break existing functionality?
| @@ -31,7 +31,8 @@ public function testOptimizelyConfigEntity() | |||
| $optConfig = new OptimizelyConfig( | |||
| "20", | |||
| ["a" => "apple"], | |||
| ["o" => "orange"] | |||
| ["o" => "orange"], | |||
| null | |||
There was a problem hiding this comment.
this shouldn't be changed.
| @@ -41,7 +42,8 @@ public function testOptimizelyConfigEntity() | |||
| $expectedJson = '{ | |||
| "revision": "20", | |||
| "experimentsMap" : {"a": "apple"}, | |||
| "featuresMap": {"o": "orange"} | |||
| "featuresMap": {"o": "orange"}, | |||
| "datafile": null | |||
| /** | ||
| * @var string datafile. | ||
| */ | ||
| private $datafile; |
There was a problem hiding this comment.
This should probably be $_datafile to be consistent with other private variable conventions. @oakbani ?
There was a problem hiding this comment.
I think the privacy convention has changed. I think @oakbani had showed me that previously. See: https://www.php-fig.org/psr/psr-12/#43-properties-and-constants
| @@ -37,6 +37,11 @@ class OptimizelyConfigService | |||
| */ | |||
| private $revision; | |||
|
|
|||
| /** | |||
| * @var string config datafile. | |||
There was a problem hiding this comment.
nit. @var string String denoting datafile.
Summary
Test plan
https://travis-ci.com/github/optimizely/fullstack-sdk-compatibility-suite/builds/181695026