search

PrestaShop / ADR

Architecture Decision Records for the PrestaShop project
11 stars 15 forks source link

0011 - Use constants for configuration variables #16

Closed JevgenijVisockij closed 2 years ago

matks commented 3 years ago

ADR added in table https://github.com/PrestaShop/ADR/blob/master/README.md

matks commented 3 years ago

@PrestaShop/prestashop-core-developers Can you check this and ask questions if you have? Else we can start the voting phase.

Progi1984 commented 3 years ago

@matks You should ping @PrestaShop/prestashop-maintainers. May we should create a meeting for all ADRs?

kpodemski commented 3 years ago

I like the idea, I'm not sure if main Configuration class is good for those statics,

Configuration::get(Configuration:PS_SHOP_NAME); `Configuration::get(ConfigKey::PS_SHOP_NAME);

use ConfigurationKeys as Key;
`Configuration::get(Key::PS_SHOP_NAME);

I don't think it's that important, I'm thinking aloud :)

PierreRambaud commented 3 years ago

I like the idea, I'm not sure if main Configuration class is good for those statics,

Configuration::get(Configuration:PS_SHOP_NAME); `Configuration::get(ConfigKey::PS_SHOP_NAME);

use ConfigurationKeys as Key;
`Configuration::get(Key::PS_SHOP_NAME);

I don't think it's that important, I'm thinking aloud :)

As I said in the PR, I'm like @kpodemski, using ConfigurationCore class isn't the best one (imho). I'm not even sure about the ConfigurationKeys class too :sweat_smile: but maybe in the adapter?

PierreRambaud commented 2 years ago

Hi @PrestaShop/prestashop-maintainers , I'm not sure it's a good idea to keep ADR for months. If it's not relevant, maybe it's time to close it? :thinking:

matks commented 2 years ago

Yes :( unfortunately ADR are a costly process as it requires time from each maintainer to explore and discuss the topic, so it's hard to be able to get everybody's attention and investment on a single topic. This topic did not make it.