Feature flags improvements #32923

boherm · 2023-06-16T08:05:07Z

Questions	Answers
Branch?	develop
Description?	This is an improvement of feature flags => See the description below or related issue.
Type?	improvement
Category?	CO
BC breaks?	no
Deprecations?	no
How to test?	See the description below. Running campaign https://github.com/jolelievre/ga.tests.ui.pr/actions/runs/5485295067
Fixed ticket?	Fixes #32698
Related PRs
Sponsor company

What I have done?

First of all, I have add DotEnv support for admin and console entry point.
Now, we have some Handlers class aim to use Feature Flags:
- Database Layer
- Env Layer
- Query Layer
- DotEnv Layer
On top of that, we have a FeatureFlagManager aim to instanciate the used handler by the feature flags, in function of priority set in type column. exemple: If a feature flag has env,query,dotenv,db in type column, this class will check if it can be possible to get a value from environnement, if not, it check if we can retrieve value by query, if not it check if we can use dotenv file, and if not, we use DB (that can be always used!)
I added type display in New & experimental features page, and the handler used is in bold for each feature flags.
I added type display in prestashop:feature-flags list command, and the handler used is surrounded by brackets.
I added a interface called FeatureFlagStateCheckerInterface to retrieve feature flag states simply.

Database Layer (type: `db`)

It's the same behaviour than before!

DotEnv Layer (type `dotenv`)

We use .env files in function of current environnement (dev, prod, etc...).
Feature flags can be edited as well in the New & experimental features page, and the Handler will localise the .env file used to set this feature flag.
The variable name in dotenv files must be: PS_FF_{UPPERCASE FEATURE FLAG NAME}.

Env Layer (type `env`)

Cannot be editable!
The variable name in env must be: PS_FF_{UPPERCASE FEATURE FLAG NAME}.

Query Layer (type `query`)

Cannot be editable and only in ModeDev!
The variable name in query string must be: PS_FF_{UPPERCASE FEATURE FLAG NAME}.

How to test?

By default, on a fresh install, we pass all feature flags in db type only.
But, you can edit PREFIX_feature_flag table to add env,query,dotenv,db on the one you want.
If you want to test Env Layer =>
- In vhost of your installation, you can add: SetEnv PS_FF_{FEATURE_FLAG_NAME} true.
- See in New & experimental features page the value of this feature flag and the handler used!
If you want to test Query Layer =>
- Add in your query string: PS_FF_{FEATURE_FLAG_NAME}=true.
- See in New & experimental features page the value of this feature flag and the handler used!
If you want to test DotEnv Layer =>
- You can create a .env file in project root, and add PS_FF_{FEATURE_FLAG_NAME}=true in it.
- See in New & experimental features page the value of this feature flag and the handler used!
- Disable this feature flag from admin page, and check if your .env file has changed.

matthieu-rolland

looks good to me 👍

M0rgan01

Nice improvements 👍

.gitignore

src/Core/FeatureFlag/Handler/AbstractHandler.php

src/Core/FeatureFlag/Handler/DbHandler.php

src/Core/FeatureFlag/Handler/DotEnvHandler.php

src/Core/FeatureFlag/Handler/EnvHandler.php

src/Core/FeatureFlag/Handler/HandlerFactory.php

src/Core/FeatureFlag/Handler/AbstractHandler.php

src/PrestaShopBundle/Command/FeatureFlagCommand.php

src/PrestaShopBundle/Entity/Repository/FeatureFlagRepository.php

kpodemski · 2023-07-07T20:32:21Z

What's going on there @jolelievre ? QA approved, no merge and further reviews? 🤔 Is it ok to merge it or not?

src/Core/FeatureFlag/Layer/DotEnvLayer.php

jolelievre · 2023-07-10T10:52:19Z

@kpodemski not ready to be merged no, I performed the QA by dev but I didn't see there was only one approval And @lartist made relevant comments since then, so still in progress

matks

👍

matks · 2023-07-10T13:16:18Z

src/Core/FeatureFlag/Layer/DbLayer.php

+     */
+    public function isReadonly(): bool
+    {
+        // It's always editable via DB layer!


what if database is read only

matks · 2023-07-10T13:23:26Z

src/Core/FeatureFlag/Layer/DotEnvLayer.php

+        if ($pathDotenv = $this->locateDotEnvFile($featureFlagName)) {
+            file_put_contents(
+                $pathDotenv,
+                preg_replace(
+                    "/({$this->getVarName($featureFlagName)})=(.*)/",
+                    "$1={$this->boolLabel($status)}",
+                    file_get_contents($pathDotenv)
+                )
+            );
+
+            return;
+        }


I think there is a few other PHP classes in prestashop that perform this operation "modify a key/value file row". Maybe this can be abstracted

private function setStatus(string $featureFlagName, bool $status): void { $this->myAwesomeFileManager->modifyLineValue($this->getVarName($featureFlagName), $this->boolLabel($status));

https://github.com/PrestaShop/PrestaShop/blob/develop/src/Adapter/Debug/DebugMode.php#L191

jolelievre · 2023-07-10T15:58:26Z

admin-dev/index.php

@@ -38,6 +39,7 @@
    define('PS_ADMIN_DIR', _PS_ADMIN_DIR_);
 }

+//define('_PS_MODE_DEV_', false);


Suggested change

//define('_PS_MODE_DEV_', false);

jolelievre · 2023-07-10T16:01:24Z

src/Core/FeatureFlag/FeatureFlagSettings.php

+    /**
+     * Type consts
+     */
+    public const TYPE_DEFAULT = 'env,query,dotenv,db';


Suggested change

public const TYPE_DEFAULT = 'env,query,dotenv,db';

public const TYPE_DEFAULT = 'env,dotenv,db';

jolelievre · 2023-07-10T16:04:20Z

src/Core/FeatureFlag/Layer/DotEnvLayer.php

+        $filesToCheck = [".env.$env.local", ".env.$env", '.env'];
+
+        foreach ($filesToCheck as $file) {
+            $path = $this->configuration->get('_PS_ROOT_DIR_') . '/' . $file;


This should be injected in the constructor and in the DI you can inject the const

jolelievre · 2023-07-10T16:07:58Z

src/Core/FeatureFlag/Layer/DotEnvLayer.php

+        if ($pathDotenv = $this->locateDotEnvFile($featureFlagName)) {
+            file_put_contents(
+                $pathDotenv,
+                preg_replace(
+                    "/({$this->getVarName($featureFlagName)})=(.*)/",
+                    "$1={$this->boolLabel($status)}",
+                    file_get_contents($pathDotenv)
+                )
+            );
+
+            return;
+        }


This is a very specific code, it's not really a good practice in the first place so moving this into a dedicated util class would incite doing this in other places We don't want to encourage this kind of thing so keeping this only in this context seems safer

Here we allow it only because it can only impact .env variables related to feature flags, since it's based on a prefix it can't modify any other future variable

jolelievre · 2023-07-10T16:13:12Z

src/Core/FeatureFlag/Layer/QueryLayer.php

+{
+    public function __construct(
+        private EnvironmentInterface $environment,
+        private ?Request $request = null


Suggested change

private ?Request $request = null

private ?RequestStack $requestStack = null

You can then use $requestStack->getMainRequest() which can be null, so in this case this layer becomes unusabled

jolelievre · 2023-07-10T16:23:01Z

tests/Unit/Core/FeatureFlag/Layer/DotEnvLayerTest.php

+{
+    private const FEATURE_FLAG_TEST = 'feature_flag_test';
+    private const VAR_FEATURE_FLAG_TEST = 'PS_FF_FEATURE_FLAG_TEST';
+    private const DOTENV_PATH = _PS_ROOT_DIR_ . '/.env.unit.local';


You can inject another folder that is part of test resources

Suggested change

private const DOTENV_PATH = _PS_ROOT_DIR_ . '/.env.unit.local';

private const DOTENV_PATH = _PS_ROOT_DIR_ . '/tests/Resources/env/.env.unit.local';

jolelievre · 2023-07-10T16:23:50Z

tests/Unit/Core/FeatureFlag/Layer/QueryLayerTest.php

+        yield ['no'];
+    }
+
+    public function createLayer(): QueryLayer


Suggested change

public function createLayer(): QueryLayer

private function createLayer(): QueryLayer

jolelievre · 2023-07-10T16:24:05Z

tests/Unit/Core/FeatureFlag/Layer/QueryLayerTest.php

+        $this->resetEnv();
+    }
+
+    private function resetEnv(): void


Can be moved at the end of the file as well

jolelievre · 2023-07-10T16:28:48Z

tests/Unit/Core/FeatureFlag/Layer/QueryLayerTest.php

+
+    private function modeDev(bool $status): void
+    {
+        static::$environment = new Environment($status, 'unit');


You should use some newly constructed mocks in each test functions instead of relying on static fields

jolelievre · 2023-07-10T16:30:20Z

tests/Unit/PrestaShopBundle/Entity/FeatureFlagTest.php

@@ -57,6 +57,8 @@ public function testAssertFeatureFlagProperties()
        $this->assertEquals('A.B.C', $featureFlag->getDescriptionDomain());
        $this->assertEquals('a b c d', $featureFlag->getLabelWording());
        $this->assertEquals('A.B.L', $featureFlag->getLabelDomain());
+        $this->assertEquals('env,query,dotenv,db', $featureFlag->getType());


Suggested change

$this->assertEquals('env,query,dotenv,db', $featureFlag->getType());

$this->assertEquals('env,dotenv,db', $featureFlag->getType());

Unless you force the value in your test entity which is also fine here

jolelievre · 2023-07-10T16:34:29Z

tests/Unit/Core/FeatureFlag/Layer/DotEnvLayerTest.php

+    private function resetEnv(): void
+    {
+        unset($_ENV[self::VAR_FEATURE_FLAG_TEST]);
+        $_ENV['SYMFONY_DOTENV_VARS'] = static::$save_dotenv_vars;


Suggested change

$_ENV['SYMFONY_DOTENV_VARS'] = static::$save_dotenv_vars;

$_ENV['SYMFONY_DOTENV_VARS'] = static::$save_dotenv_vars;

unlink(self::DOTENV_PATH);

jolelievre · 2023-07-10T16:35:08Z

tests/Unit/Core/FeatureFlag/Layer/DotEnvLayerTest.php

+{
+    private const FEATURE_FLAG_TEST = 'feature_flag_test';
+    private const VAR_FEATURE_FLAG_TEST = 'PS_FF_FEATURE_FLAG_TEST';
+    private const DOTENV_PATH = _PS_ROOT_DIR_ . '/.env.unit.local';


Suggested change

private const DOTENV_PATH = _PS_ROOT_DIR_ . '/.env.unit.local';

private const DOTENV_PATH = _PS_ROOT_DIR_ . '/tests/Resources/env/.env.unit.local';

jolelievre · 2023-07-10T16:37:47Z

.env

@@ -0,0 +1 @@
+PS_FF_PRODUCT_PAGE_V2=true


Should be empty (maybe add a comment to explain why this file is added following the Symfony convention, but we have no default values to force for now)

I'm more infavor of using a .env.dist file though, it follows a convention that is widely used in many other projects so I don't really understand why we should use .env as our dist file
Especially since DotEnv does handle the .env.dist file as its default fallback

Ok after comparing a few projects it seems like Symfony projects mostly follow the .env convention It feels kind of weird but I guess we'd better follow the framework conventions

jolelievre

Thanks @boherm

jolelievre · 2023-07-11T11:10:46Z

Validated by QA by dev waiting for https://github.com/jolelievre/ga.tests.ui.pr/actions/runs/5519072374 before merge

boherm requested a review from a team as a code owner June 16, 2023 08:05

prestonBot added develop Branch Improvement Type: Improvement labels Jun 16, 2023

boherm force-pushed the #32698-feature-flags-improvements branch 3 times, most recently from 6740a71 to 4aab36d Compare June 16, 2023 10:32

matthieu-rolland previously approved these changes Jun 16, 2023

View reviewed changes

M0rgan01 approved these changes Jun 19, 2023

View reviewed changes

lartist reviewed Jun 19, 2023

View reviewed changes

boherm dismissed matthieu-rolland’s stale review via 8cc7627 June 20, 2023 14:10

boherm marked this pull request as draft June 22, 2023 16:45

boherm added 2 commits June 23, 2023 17:36

Add DotEnv support in console and admin entry points

1b4a394

Add type in feature flags entity and installed data

303e0df

boherm force-pushed the #32698-feature-flags-improvements branch 2 times, most recently from 73fefbc to f320ce1 Compare June 23, 2023 15:38

boherm marked this pull request as ready for review June 23, 2023 15:39

boherm added 2 commits June 23, 2023 17:57

Add Handlers for feature flags

928e5cc

Implement Handler to deal with feature flags

ac5ac07

boherm force-pushed the #32698-feature-flags-improvements branch from f320ce1 to a622bb4 Compare June 23, 2023 16:00

boherm closed this Jun 23, 2023

boherm reopened this Jun 23, 2023

boherm force-pushed the #32698-feature-flags-improvements branch 4 times, most recently from b616f07 to 7c01599 Compare June 23, 2023 16:51

boherm added 2 commits June 23, 2023 19:17

Use FeatureFlagService instead of FeatureFlagRepository directly

d8cf722

Fix tests

15ecae8

boherm force-pushed the #32698-feature-flags-improvements branch from 7c01599 to 15ecae8 Compare June 23, 2023 17:17

boherm requested review from M0rgan01 and lartist June 23, 2023 17:46

jolelievre removed the QA ✔️ Status: check done, code approved label Jul 7, 2023

FabienPapet reviewed Jul 8, 2023

View reviewed changes

src/Core/FeatureFlag/Layer/DotEnvLayer.php Outdated Show resolved Hide resolved

matks reviewed Jul 10, 2023

View reviewed changes

boherm dismissed jolelievre’s stale review via 1073385 July 10, 2023 14:39

boherm force-pushed the #32698-feature-flags-improvements branch 4 times, most recently from c479cca to 4aa6c6c Compare July 10, 2023 15:48

jolelievre reviewed Jul 10, 2023

View reviewed changes

boherm force-pushed the #32698-feature-flags-improvements branch from 4aa6c6c to 2895629 Compare July 10, 2023 17:08

boherm added 2 commits July 10, 2023 19:10

Fix comments

9a8c245

Add tests

21b71ab

boherm force-pushed the #32698-feature-flags-improvements branch from 2895629 to 21b71ab Compare July 10, 2023 17:10

jolelievre approved these changes Jul 10, 2023

View reviewed changes

boherm requested review from FabienPapet, matks, jolelievre, lartist and M0rgan01 July 10, 2023 17:32

lartist approved these changes Jul 11, 2023

View reviewed changes

jolelievre approved these changes Jul 11, 2023

View reviewed changes

jolelievre added the QA ✔️ Status: check done, code approved label Jul 11, 2023

jolelievre merged commit 2f9be0d into PrestaShop:develop Jul 11, 2023
20 checks passed

boherm deleted the #32698-feature-flags-improvements branch July 11, 2023 13:16

matks mentioned this pull request Nov 30, 2023

Notable changes for PrestaShop 9 PrestaShop/docs#1658

Open

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature flags improvements #32923

Feature flags improvements #32923

boherm commented Jun 16, 2023 •

edited

matthieu-rolland left a comment

M0rgan01 left a comment

kpodemski commented Jul 7, 2023

jolelievre commented Jul 10, 2023

matks left a comment

matks Jul 10, 2023

matks Jul 10, 2023

jolelievre Jul 10, 2023

jolelievre Jul 10, 2023

jolelievre Jul 10, 2023

jolelievre Jul 10, 2023

jolelievre Jul 10, 2023

jolelievre Jul 10, 2023

jolelievre Jul 10, 2023

jolelievre Jul 10, 2023

jolelievre Jul 10, 2023

jolelievre Jul 10, 2023

jolelievre Jul 10, 2023

jolelievre Jul 10, 2023

jolelievre Jul 10, 2023

jolelievre Jul 10, 2023

jolelievre Jul 10, 2023

jolelievre Jul 10, 2023

jolelievre Jul 10, 2023

jolelievre left a comment

jolelievre commented Jul 11, 2023

	public const TYPE_DEFAULT = 'env,query,dotenv,db';
	public const TYPE_DEFAULT = 'env,dotenv,db';

	private ?Request $request = null
	private ?RequestStack $requestStack = null

	private const DOTENV_PATH = _PS_ROOT_DIR_ . '/.env.unit.local';
	private const DOTENV_PATH = _PS_ROOT_DIR_ . '/tests/Resources/env/.env.unit.local';

	public function createLayer(): QueryLayer
	private function createLayer(): QueryLayer

	$this->assertEquals('env,query,dotenv,db', $featureFlag->getType());
	$this->assertEquals('env,dotenv,db', $featureFlag->getType());

	$_ENV['SYMFONY_DOTENV_VARS'] = static::$save_dotenv_vars;
	$_ENV['SYMFONY_DOTENV_VARS'] = static::$save_dotenv_vars;
	unlink(self::DOTENV_PATH);

		@@ -0,0 +1 @@
		PS_FF_PRODUCT_PAGE_V2=true

Feature flags improvements #32923

Feature flags improvements #32923

Conversation

boherm commented Jun 16, 2023 • edited

What I have done?

Database Layer (type: db)

DotEnv Layer (type dotenv)

Env Layer (type env)

Query Layer (type query)

How to test?

matthieu-rolland left a comment

Choose a reason for hiding this comment

M0rgan01 left a comment

Choose a reason for hiding this comment

kpodemski commented Jul 7, 2023

jolelievre commented Jul 10, 2023

matks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jolelievre left a comment

Choose a reason for hiding this comment

jolelievre commented Jul 11, 2023

boherm commented Jun 16, 2023 •

edited

Database Layer (type: `db`)

DotEnv Layer (type `dotenv`)

Env Layer (type `env`)

Query Layer (type `query`)