From baed859a6b6a52f328c6d4091ab3d9b0344b2e3c Mon Sep 17 00:00:00 2001 From: Jevgenij Visockij Date: Tue, 5 Jan 2021 12:01:29 +0200 Subject: [PATCH 1/5] Adding ADR for use of constants for Configuration names --- ...-constantas-for-configuration-variables.md | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+) create mode 100644 0011-use-constantas-for-configuration-variables.md diff --git a/0011-use-constantas-for-configuration-variables.md b/0011-use-constantas-for-configuration-variables.md new file mode 100644 index 0000000..6c2c9d9 --- /dev/null +++ b/0011-use-constantas-for-configuration-variables.md @@ -0,0 +1,34 @@ +# 11. Use constants for configuration variables + +Date: 2020-01-05 + +## Status + +In discussion + +## Context + +Configuration names in PrestaShop currently are just hardcoded. It makes it harder to figure out +which configurations are available, also leaves possibility for mistyping configuration name without noticing. + +## Decision + +Use constants in Configuration class to define all the configuration names. + +### Why use Configuration class +It's already exists for all purposes concerning usage of configurations and creation of the new class just to store +constants is not worthwhile in this case. + +## First implementation + +Will update once PR to PrestaShop is created +## Consequences + +What becomes easier : + +- You can't miss-write configuration name +- Using constants uses less memory than plain string +- You can change the name of constant without considering it as breaking change +- It's easier to refactor +- When all configuration names are in one place, it's easier to fine one you are looking for +- Allows deprecating configurations From d6c7d5501fc2d7a25c03a63d2ebb0b46e0f337e9 Mon Sep 17 00:00:00 2001 From: Jevgenij Visockij Date: Tue, 5 Jan 2021 12:05:14 +0200 Subject: [PATCH 2/5] Added first implementation --- 0011-use-constantas-for-configuration-variables.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/0011-use-constantas-for-configuration-variables.md b/0011-use-constantas-for-configuration-variables.md index 6c2c9d9..fba5f80 100644 --- a/0011-use-constantas-for-configuration-variables.md +++ b/0011-use-constantas-for-configuration-variables.md @@ -21,7 +21,8 @@ constants is not worthwhile in this case. ## First implementation -Will update once PR to PrestaShop is created +https://github.com/PrestaShop/PrestaShop/pull/22672 + ## Consequences What becomes easier : From bed9954da3dfd7edcad63ec18bc2c025b14945aa Mon Sep 17 00:00:00 2001 From: Jevgenij Visockij Date: Tue, 5 Jan 2021 12:07:06 +0200 Subject: [PATCH 3/5] Fixed grammar --- 0011-use-constantas-for-configuration-variables.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/0011-use-constantas-for-configuration-variables.md b/0011-use-constantas-for-configuration-variables.md index fba5f80..d06bc9c 100644 --- a/0011-use-constantas-for-configuration-variables.md +++ b/0011-use-constantas-for-configuration-variables.md @@ -8,15 +8,15 @@ In discussion ## Context -Configuration names in PrestaShop currently are just hardcoded. It makes it harder to figure out +Configuration names in PrestaShop are used as hardcoded strings. It makes it harder to figure out which configurations are available, also leaves possibility for mistyping configuration name without noticing. ## Decision -Use constants in Configuration class to define all the configuration names. +Use constants in Configuration class to define all the configuration names and then use them in code instead of plain strings. ### Why use Configuration class -It's already exists for all purposes concerning usage of configurations and creation of the new class just to store +It already exists for all purposes concerning usage of configurations. Creation of the new class just to store constants is not worthwhile in this case. ## First implementation From 1d05e1f39c0c907b174d1768416c3d095b18b554 Mon Sep 17 00:00:00 2001 From: Jevgenij Visockij Date: Tue, 5 Jan 2021 16:53:39 +0200 Subject: [PATCH 4/5] Added drawbacks --- 0011-use-constantas-for-configuration-variables.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/0011-use-constantas-for-configuration-variables.md b/0011-use-constantas-for-configuration-variables.md index d06bc9c..9a611a5 100644 --- a/0011-use-constantas-for-configuration-variables.md +++ b/0011-use-constantas-for-configuration-variables.md @@ -33,3 +33,7 @@ What becomes easier : - It's easier to refactor - When all configuration names are in one place, it's easier to fine one you are looking for - Allows deprecating configurations + +What becomes harder : +- You need to include(use or require) Configuration class everywhere, you want to use Configurations +- Need to replace Configuration names with constants for this to be useful, which will require some work and testing From 86258133b4a9d2c8c9bdf6d8ddbd0856c1cb706d Mon Sep 17 00:00:00 2001 From: Jevgenij Visockij Date: Tue, 5 Jan 2021 17:06:49 +0200 Subject: [PATCH 5/5] Updated the description --- ...-constantas-for-configuration-variables.md | 23 +++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/0011-use-constantas-for-configuration-variables.md b/0011-use-constantas-for-configuration-variables.md index 9a611a5..9f96ca7 100644 --- a/0011-use-constantas-for-configuration-variables.md +++ b/0011-use-constantas-for-configuration-variables.md @@ -8,13 +8,27 @@ In discussion ## Context -Configuration names in PrestaShop are used as hardcoded strings. It makes it harder to figure out -which configurations are available, also leaves possibility for mistyping configuration name without noticing. +Configuration names in PrestaShop are used as hardcoded strings. +For example: + + PS_SSL_ENABLED + PS_SHOP_ENABLE + +It makes it harder to figure out which configurations are available, also leaves possibility for mistyping configuration name without noticing. + +If some Configuration is no longer used, there is no proper process to deprecated it, leading to configurations like + + PS_CONFIGURATION_AGREMENT + +cluttering database, but not having any actual use. ## Decision -Use constants in Configuration class to define all the configuration names and then use them in code instead of plain strings. - +Use constants in Configuration class to define all the configuration names and then use them in code instead of plain strings. +So in code we would have `Configuration::updateValue(Configuration::PS_SSL_ENABLED, 1);` instead of `Configuration::updateValue('PS_SSL_ENABLED', 1);` + +This would also allow for you to have a documentation of sorts for configuration names, by having them all in one place. + ### Why use Configuration class It already exists for all purposes concerning usage of configurations. Creation of the new class just to store constants is not worthwhile in this case. @@ -33,6 +47,7 @@ What becomes easier : - It's easier to refactor - When all configuration names are in one place, it's easier to fine one you are looking for - Allows deprecating configurations +- Allows to document Configurations in code What becomes harder : - You need to include(use or require) Configuration class everywhere, you want to use Configurations