PES-1251, PES-2068, PES-2080 Default weight and packet dimensions #78
Conversation
media/admin/com_virtuemart/views/zasilkovna/tmpl/default_config.php
Outdated
Show resolved
Hide resolved
zemekoule
changed the title
PES-2068 CR refactor - introduce constants PES-2068 CR fix - unused getters removed PES-2068 CR fix - format table using css PES-2068 CR fix - typehints & annotations PES-2068 CR fix - introduce updateZasilkovnaConfig PES-2068 CR fix - introduce configuration defaults PES-2068 final minor CR fixes PES-2068 CS fixes and minor improvement PES-2068 Use ConfigValidator::KEY_PAYMENT_METHOD_PREFIX in template PES-2068 CR - final fix for strange VM null handling
Mrakor
force-pushed
the
PES-1251_default_weight_and_packet_dimensions
branch
from
137ff3e to
9b8f1d3
Compare
media/admin/com_virtuemart/models/zasilkovna_src/VirtueMartModelZasilkovna/ShipmentMethod.php
Outdated
Show resolved
Hide resolved
media/admin/com_virtuemart/views/zasilkovna/tmpl/default_config.php
Outdated
Show resolved
Hide resolved
...in/com_virtuemart/models/zasilkovna_src/VirtueMartModelZasilkovna/ConfigurationValidator.php
Outdated
Show resolved
Hide resolved
...in/com_virtuemart/models/zasilkovna_src/VirtueMartModelZasilkovna/ConfigurationValidator.php
Outdated
Show resolved
Hide resolved
...in/com_virtuemart/models/zasilkovna_src/VirtueMartModelZasilkovna/ConfigurationValidator.php
Outdated
Show resolved
Hide resolved
...in/com_virtuemart/models/zasilkovna_src/VirtueMartModelZasilkovna/ConfigurationValidator.php
Outdated
Show resolved
Hide resolved
...in/com_virtuemart/models/zasilkovna_src/VirtueMartModelZasilkovna/ConfigurationValidator.php
Outdated
Show resolved
Hide resolved
introduce ConfigConstants introduce ConfigurationValidator::normalize
zemekoule
changed the title
| */ | ||
| private function updateZasilkovnaConfig(array $data): FlashMessage | ||
| { | ||
| $configStorage = new VirtueMartModelZasilkovna\ConfigSessionStorage(JFactory::getSession(), 'packeteryConfig'); |
There was a problem hiding this comment.
namespace by měl být uvnitř třídy ConfigSessionStorage - je součástí zapouzdření ukládací logiky
a pojmenuj $configSessionStorage, ať je níže jasnější, kam ukládáme a kde mažeme (dtto ve view.html.php)
| $this->set(self::ID, self::KEY, $data); | ||
| } | ||
|
|
||
| public function flush(): void |
There was a problem hiding this comment.
raději clear() nebo removeAll()
flush() u Doctrine znamená uložit změny, radši bych zvolil jednoznačnější pojmenování
|
|
||
| $normalizedData = []; | ||
| $normalizedData[ConfigConstants::KEY_API_PASS] = $this->formData[ConfigConstants::KEY_API_PASS]; | ||
| $normalizedData[ConfigConstants::KEY_ESHOP_LABEL] = $this->formData[ConfigConstants::KEY_ESHOP_LABEL] ?? ConfigConstants::CONFIG_DEFAULTS[ConfigConstants::KEY_ESHOP_LABEL]; |
There was a problem hiding this comment.
Používání defaultního labelu je zde zbytečné. Odeslaný formulář má hodnotu vždy (byť třeba prázdný string), takže kontrola přes ?? je zbytečná. Odstraň, prosím, default (+ viz naše diskuse o defaultech jako lokální proměnné v install).
| private array $errors = []; | ||
|
|
||
| /** | ||
| * @param array<string, mixed> $formData |
There was a problem hiding this comment.
hodnoty mohou být jen stringy, je to POST
|
|
||
| private function validateDimensions(): void { | ||
| foreach (self::DIMENSIONS as $field => $label) { | ||
| if ((!empty($this->formData[$field]) || $this->formData[$field] === '0') |
There was a problem hiding this comment.
jednodušší testovat jen !== ''
místo !empty() || === '0'
| $size[$dimension] = $order[$dimension]; | ||
| } | ||
|
|
||
| if ($order['length']) { |
There was a problem hiding this comment.
tahle podmínka patří nad celé sestavování $size
| /** | ||
| * @param string $name | ||
| * @param mixed $default | ||
| * @return array|mixed|string |
There was a problem hiding this comment.
@return mixed stačí - a proto úplně odstranit celý PHPDoc, protože nepřináší nic navíc
| * @param mixed $default | ||
| * @return array|mixed|string | ||
| */ | ||
| public function getFormValue(string $name, mixed $default = ''): mixed |
There was a problem hiding this comment.
mixed $default je v našem případě zbytečně široké
| <?php | ||
| namespace VirtueMartModelZasilkovna; | ||
|
|
||
| class ConfigurationValidator |
There was a problem hiding this comment.
Obecně se mi tyto metody špatně čtou, řádky jsou dlouhé, ify obsahují hodně logických prvků.
Tj. aspoň zalamovat, ale ještě lépe vyčleňovat kusy logiky do samostatně pojmenovaných proměnných např. $useDefaultDimensions -> if ($useDefaultDimension && ...)
No description provided.