-
Notifications
You must be signed in to change notification settings - Fork 15
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
0011 - Use constants for configuration variables #16
Changes from all commits
baed859
d6c7d55
bed9954
1d05e1f
8625813
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,54 @@ | ||
# 11. Use constants for configuration variables | ||
|
||
Date: 2020-01-05 | ||
|
||
## Status | ||
|
||
In discussion | ||
|
||
## Context | ||
|
||
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. | ||
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. | ||
|
||
## First implementation | ||
|
||
https://github.com/PrestaShop/PrestaShop/pull/22672 | ||
|
||
## Consequences | ||
|
||
What becomes easier : | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To be honest, we must also list the drawbacks 😄 . This means for example we need to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added 2 drawbacks, couldn't think of more at the moment. |
||
|
||
- 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 | ||
- Allows to document Configurations in code | ||
|
||
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, configuration names don't always match the names of configuration form fields, for example, form field
enable_shop
is linked to the configuration name PS_SHOP_ENABLE. (and not PS_ENABLE_SHOP as we would intuitively guess).Using constants would make it less error-prone.