Vlastní event systém nahrazen za PSR-14 #96
Conversation
|
Tohle je poslední změna zasahující do funkčnosti. Plánuji dále už jen přidat logování(#70), update dokumentace a možná trochu pročistit testy. Nechávám PRka otevřené do konce příštího týdne. Pokud nebudou žádné připomínky, mergnu je a vydám Pokud má někdo nějaké nápady co do 3.0 ještě přidat, teď je vhodná chvíle se o ně podělit. |
|
Ahoj, nechceš spíš prosím udělat PR se vším, co teda plánuješ do 3.0? Jakože já se asi nevyznám v tomhle kódu natolik, abych dělal nějaký review tady toho PR, ale kdybych měl plánovaný change log pro 3.0, tak k tomu třeba budu mít, co říct 🙂 Díky, |
src/Wsdl/Event/RequestPreEvent.php
Outdated
| /** | ||
| * @return array<int, mixed> | ||
| */ | ||
| public function &getArgs(): array |
There was a problem hiding this comment.
@JindrichPilar Těm referencím bych se radši vyvaroval. K čemu mají být? Pokud to má být způsob, jak upravit v subscriberu ten daný event, tak je to IMO dost messy řešení. Mnohem lepší je v tu chvíli použít vlastní dekorátor kolem servisy (tím, že dokonce nabízíme i AbstractDecorator je to dost snadný způsob), případně vlastní implementaci webservice. Tohle totiž může vést k nehezkým bugům.
There was a problem hiding this comment.
použít vlastní dekorátor kolem servisy
Ten se nedostane koptionsainputHeaders. Bez hacku s reflection.
Tohle totiž může vést k nehezkým bugům.
Bylo to myšleno jako expert only feature s velkým varováním v dokumentaci.
Vzhledem k #97 jsem to ale odstranil, nejsem si jistý jak moc by to bylo kompatibilní.
There was a problem hiding this comment.
Vnímám tam jen dva problémy:
-
Serializace
Minimálně bych ještě zvážil, jestli vůbec serializaci podporovat potřebujeme? IMO to pro většinu use-casů v PHP aplikacích napojených na Skautis není potřeba, PSR-14 zároveň serializovatelnost nevyžaduje. Přijde mi, že to je dost kódu pro funkcionalitu, která nemusí být potřeba. -
Reference pro úpravu requestu
Tohle IMO přinese víc problémů než užitku, pokud vážně potřebujeme přidat možnost upravovat request v listeneru (IMO nepotřebujeme), tak by to mělo být přes normální metody (alaRequestPreEvent::modifyRequestArgs($args)). To PSR takovou obousměrnou komunikaci sice umožňuje, ale IMO u nás existuje pro tento účel lepší řešení (třeba vlastní decorator kolem webservisy)
Jinak kromě těch pár komentářů nemám žádné výtky 👍
src/Wsdl/Event/RequestFailEvent.php
Outdated
| * | ||
| * @var string | ||
| */ | ||
| private $exceptionClass = ''; |
There was a problem hiding this comment.
Tady není potřeba mít výchozí hodnotu, protože v konstruktoru ji vždy přepíšeš + ten komentář IMO není pravda, protože je to inicializováno vždy.
src/Wsdl/Event/RequestFailEvent.php
Outdated
| * | ||
| * @var string | ||
| */ | ||
| private $exceptionString = ''; |
There was a problem hiding this comment.
Tady není potřeba mít výchozí hodnotu, protože v konstruktoru ji vždy přepíšeš + ten komentář IMO není pravda, protože je to inicializováno vždy.
src/Wsdl/Event/RequestPostEvent.php
Outdated
|
|
||
|
|
||
| /** | ||
| * @var float Doba trvani pozadvku |
There was a problem hiding this comment.
Můžeme doplnit, o jakou jednotku se jedná (vteřiny, ms, ...)?
src/Wsdl/Event/RequestPostEvent.php
Outdated
| */ | ||
| public function unserialize($data): void | ||
| { | ||
| $data = unserialize($data, [self::class, stdClass::class]); |
There was a problem hiding this comment.
Viz https://www.php.net/manual/en/function.unserialize.php#refsect1-function.unserialize-parameters
| $data = unserialize($data, [self::class, stdClass::class]); | |
| $data = unserialize($data, ['allowed_classes' => [self::class]]); |
| use Serializable; | ||
| use stdClass; | ||
|
|
||
| class RequestPostEvent implements Serializable |
There was a problem hiding this comment.
Potřebujeme tady implementovat Serializable? Vidím, že se všechny properties standardně serializují.
There was a problem hiding this comment.
Není to vyloženě potřeba. Já preferuji explicitnost spíš než minimální počet řádků. Ale pokud ti to vyloženě vadí, tak to odstraním, zas tolik na tom netrvám. Nepamatuji si totiž proč se to původně implementovalo. Myslím že to dělalo problém v nějakém debug baru v Nette/Symfony.
|
|
||
| use Serializable; | ||
|
|
||
| class RequestPreEvent implements Serializable |
There was a problem hiding this comment.
Potřebujeme tady implementovat Serializable? Vidím, že se všechny properties standardně serializují.
|
@JindrichPilar Jinak k tvé poznámce o tom, co bysme v 3.0 ještě uvítali - za mě #97 (nemám problém to i doimplementovat) |
@marekdedic asi nerozumím co myslíš. Do changelogu přídávám krátký popis neinterních změn vždy v každém PR. Takhle vypadá changelog už mergnutých změn. A mělo by se tam objevit ještě: * Vlastní event dispatcher byl nahrazen [PSR-14](https://www.php-fig.org/psr/psr-14/) - zpětně nekompatibilní.
* DateTime bylo nahrazeno DateTimeImmutable a DateTimeInterface
* Místo prázdného objektu typu ``\stdClass`` se nyní vrací null pro jeden výsledek, nebo prazdne pole pokud se jednalo o dotaz vracející kolekci.
* Možnost logování pomocí PSR-3 |
|
Ahoj, Díky, |
No description provided.