search

skaut / Skautis

PHP knihovna pro připojení do skautISu
BSD 3-Clause "New" or "Revised" License
12 stars 11 forks source link

Nahrazení SoapClient za nějakou rozumnější abstrakci #97

Open fmasa opened 4 years ago

fmasa commented 4 years ago

Rád bych navrhl pro verzi 3.0 výměnu Soap klienta. Je to něco, co minimálně ve svých projektech mám v plánu dělat, a tedy nabízím implementaci přímo tady. Rád bych využil buď Guzzle/PSR-7 (osobně mi přijde z uživatelského pohledu jednodušší mít závislost přímo na Guzzle), nebo např. meng-tian/async-soap-guzzle

Motivace

JindrichPilar commented 4 years ago

výměnu Soap klienta

Nerad bych kompletní výměnu, ale preferoval bych umožnit alternativu.

Přidal bych do WebServiceInterface něco jako batchCall, s tím že současná implementace by je pustila po sobě. Uživatel by použil jinou implementaci přes WebServiceFactory.

mít závislost přímo na Guzzle

Souhlasím, rozhodně bych se vyhnul jakékoliv ne-mainstreamove knihovne. Pokud to licence umožní, tak můžeme třeba část jiné knihovny vyloženě převzít.

Také bych se zamyslel nakolik se vázat přímo na Guzzle a nakolik využít PSR-7/PSR-17/PSR-18.

fmasa commented 4 years ago

Určitě IMO dává smysl mít vazbu spíše na PSR-7 klienta. Spíš jsem tím Guzzlem myslel to, že IMO by se hodilo přidat závislost na nějakém konkrétním klientu, ať uživateli stačí přidat závislost na skautis knihovně a nemusí hledat ještě nějakou implementaci HTTP klienta. Ale i varianta s doinstalováním je asi ok.

Náhradu jsem navrhoval vesměs kvůli tomu, že by nebylo potřeba udržovat dvě alternativní řešení. Pokud totiž nebude nějaký výrazný rozdíl v performance, tak IMO nedává moc smysl mít dvě různé implementace. Zvlášť pokud jedna z nich bude mít podmnožinu featur.

fmasa commented 4 years ago

Jinak i převzetí části knihovny asi dává smysl. Třeba ta, co jsem posílal, je pod MIT licencí, což by neměl být problém.

fmasa commented 4 years ago

Ještě teda koukám, že PSR-18 asi není pro naše účely příliš použitelné - jedná se pouze o sync API.

JindrichPilar commented 4 years ago

hodilo přidat závislost na nějakém konkrétním klientu nemusí hledat ještě nějakou implementaci HTTP klienta

Tim ale uzivateli omezime verze HTTP klienta na ty ktere podporuje Skautis. A zaroven tim omezime verzi PHP na ty ktere podporuje ta knihovna a jeji zavislosti.

SoapClient, jako officialni PHP extension, je (AFAIK) podporovany na vsech verzich PHP. A s PSR-18 (i kdyz sync) by mel moznost si vybrat jakou implementaci chce a ktera mu pobezi, protoze ten interface se asi menit nebude.

Od toho aby nemusel hledat je suggest v composer.json.

nebylo potřeba udržovat dvě alternativní řešení. Pokud totiž nebude nějaký výrazný rozdíl v performance

Tady mi slo o SOAP protokol, kdyby byl nejaky problem s nasi implementaci, mohl by uzivatel fall-backnout na SoapClient.

Pokud bychom chteli SoapClient vyhodit uplne, chtelo by to opravdu silnou integration test suite s test SkautISem.

fmasa commented 4 years ago

Já jsem 100% pro interoperabilitu, ale AFAIK teď prostě žádný standard pro asynchronní HTTP klienty není.

Tim ale uzivateli omezime verze HTTP klienta na ty ktere podporuje Skautis. A zaroven tim omezime verzi PHP na ty ktere podporuje ta knihovna a jeji zavislosti.

Tohle nevnímám jako problém. Ve chvíli, kdy vyjde nová verze major této knihovny, to povede k nutnosti přidání podpory do tohoto balíčku. Na druhou stranu teď vychází Guzzle 7. Guzzle 6 tu byl 5 let a zatím je stále udržovaný. Jeden PR za 5 let IMO není tak velký overhead.

Ohledně podporované verze PHP - jedná se o udržovanou knihovnu a troufnu si tvrdit, že na nové PHP verze bude připravena dříve než my. Značná část projektů, které skautis/skautis využívají IMO poběží na Lebedě, kde novou verzi PHP dostáváme s několikaměsíčním zpožděním.

Nehledě na to, že je naprosto ok dále používat verzi 2.x.

Tady mi slo o SOAP protokol, kdyby byl nejaky problem s nasi implementaci, mohl by uzivatel fall-backnout na SoapClient.

S ponecháním SoapClient varianty ve výsledku nemám problém. Jak píšu, IMO je to zbytečná práce navíc, ale na druhou stranu to řešení se SoapClient už existuje a ještě ho nějak udržovat je IMO ok.

Další varianta je, že bych to naimplementoval jako samostatný balíček a pokud se to osvědčí, tak můžeme přidat přímo do skautis/skautis v nějaké další minor verzi. V tu chvíli by ale minimálně stálo za to mít ve WebserviceInterface in nějakou tu metodu pro async (zde se dostáváme k problému, že ani promises nejsou v rámci PHP nijak standardizované).

JindrichPilar commented 4 years ago

standard pro asynchronní HTTP klienty není

Pravda.

Tohle nevnímám jako problém. Ve chvíli, kdy vyjde nová verze major této knihovny, to povede k nutnosti přidání podpory do tohoto balíčku. Na druhou stranu teď vychází Guzzle 7. Guzzle 6 tu byl 5 let a zatím je stále udržovaný. Jeden PR za 5 let IMO není tak velký overhead.

Guzzle 7 ma zavilslost symfony/polyfill-intl-idn: 1.17.0, tedy neumoznuje ani jinou patch version.

Na druhou stranu teď vychází Guzzle 7

Takze bychom to vytvorili na tu novou verzi, i kdyz v bete?

naimplementoval jako samostatný balíček

Spis bych nechal SoapClient jako default, Guzzle 7 jako dev dependency + suggest. A v Skautis::getInstance mel logiku ktera v pripade ze Guzzle (7) je pritomen pouzije GuzzleWebServiceFactory.

V tu chvíli by ale minimálně stálo za to mít ve WebserviceInterface in nějakou tu metodu pro async (zde se dostáváme k problému, že ani promises nejsou v rámci PHP nijak standardizované)

Pravda, to je ted potreba vymyslet. Napadji me tyto 3 moznosti:

A) Nejjednodussi by byl batch reqest. Dostane pole requestu ['optional_key_A' => $requestA] a vrati pole odpovedi ['optional_key_A' => $reponseForRequestA]. Coz ale neumozni zprocesovani driv nez se dostane odpoved na vsechny dotazy. Nadruhou stranu je jednoduchy na pouziti i pro zacatecnika.

B) Vytvorit vlastni promise interface, co nejjednodussi, idealne jen then a wait metody. A to z async vracet. Ve chvili kdy bude PSR na promise, da se tim nejsnadneji nahradit.

C) Pouzit Guzzle promise knihovnu, je nejpouzivanejsi. Moznost stejnych problemu jako prima zavislost na celem Guzzle.

fmasa commented 4 years ago

Pravda, to je ted potreba vymyslet. Napadji me tyto 3 moznosti: Nejvíc se mi líbí varianta C, ta konkrétní knihovna je stable a už dlouho běžící na jedné major verzi.

Spis bych nechal SoapClient jako default, Guzzle 7 jako dev dependency + suggest. A v Skautis::getInstance mel logiku ktera v pripade ze Guzzle (7) je pritomen pouzije GuzzleWebServiceFactory. Ale i A by IMO mohlo dobře fungovat, pokud by byl nějaký objekt pro request (místo předávání nějak definovaného asociativního pole - přeci jen potřebujeme předávat název akce + argumenty)

Tady bych osobně fakt upřednostnil samostatný balíček, protože:

*Pominu to, že nic jako optional dependency v pravém slova smyslu neexistuje, protože pokud přidáme implementaci proti Guzzle 6, tak implicitně závisíme na konkrétním API, což je IMO výrazně problematičtější varianta než explicitní závislost.

JindrichPilar commented 4 years ago

Ok, tedy jaky bude dalsi postup? Dava mi smysl toto:

marekdedic commented 4 years ago

A je opravdu potřeba si na sebe šít bič v podobě podpory 2 implementací? Jakože co to přidává oproti možnosti tam prostě hodit guzzle? Google API client to tak myslím taky má. Navíc verze 3 stejně zvedá nejnižší podporovanou verzi PHP, takže nějaký odkaz na možné nekompatibility mi přijde lichý, protože to stejně uživatelé budou muset řešit.

Za sebe async API vítám, myslím, že by to měl být výchozí způsob práce s knihovnou.

JindrichPilar commented 4 years ago

Navíc verze 3 stejně zvedá nejnižší podporovanou verzi PHP, takže nějaký odkaz na možné nekompatibility mi přijde lichý, protože to stejně uživatelé budou muset řešit.

Nejde jen o verzi PHP, ale i verze knihoven. Tech na kterych zavisi tahle knihovna a jejich zavislostech a tak dale.

Tedy pokud by Skautis 3.x pozadoval Guzzle 7, nesel by pouzit v projektu ktery pozaduje Guzzle 6. Napriklad tebou zmineny Google API client ktery pozaduje ~5.3.1||~6.0. Nebo treba mhujer/fio-api-php ktere zavisi na Guzzle 6 a ktery pouziva Skautske Hospodareni a SRS.

Pricemz Guzzle 6 funguje i na PHP 7.4 tak nikoho prilis netlaci upgradovat.

Dalsi priklad jak uz jsem psal drive tak Guzzle 7 dev verze vyzaduje symfony/polyfill-intl-idn: 1.17.0. Presne 1.17.0. Pokud jina knihovna ma treba presne 1.16.0, nebo 1.16.* apodobne, bude mit uzivatel problem.

Google API client to tak myslím taky má

AFAIK ten implementuje primo HTTP API, nema imeplmentaci SOAP protokolu. Pro spolehlive pouziti by bylo nad Guzzle potreba implementovat SOAP 1.2 part 1, a WSDL 2.0 part 1 a mozna i SOAP 1.2 part 2 a WSDL 2.0 part 2. Pokud v tom udelame chybu, uzivatel ma problem.

PHP nabizi leta otestavanou implementaci pro WSDL/SOAP, dostupnou pro vsechny pouzivane verze PHP. Tedy neomezuje pouzite knihovny ani verze PHP.

A je opravdu potřeba si na sebe šít bič v podobě podpory 2 implementací?

Jedine co by bylo potreba delat duplikatne je implementace WebServiceInterface a WebServiceFactoryInterface. Pricemz soucasna implementaceWebService se za posledni roky moc nemenila a roky nejspis zase nebude. Tedy nechat tam podporu pro PHP SoapClienta nas moc casu stat nebude.

marekdedic commented 4 years ago

No, Guzzle 7 není myslím zatím moc stable, takže s ničím výrazým by tam s šestkou konflikt nebyl... V budoucnu podpora 6 i 7 nevím, jak moc je reálná...

Implementovat SOAP nad Guzzlem by se pro jeho podporu muselo tak, jako tak, ať už bude jedinou možností, nebo tam bude i ten SoapClient.

Ale pokud to je opravdu tak jednoduché, jak píšeš, tak to pro vývoj není asi problém. Spíš se bojím, aby to nebylo matoucí pro uživatele, že mají na výběr 2 možnosti, jak tu knihovnu použít...

fmasa commented 4 years ago

Ok, tedy jaky bude dalsi postup?

Zatím bych jen přidal metodu do WebServiceInterface a tu naší implementaci pro Promise. Zatím bych stejně nastřelil nějakou quick&dirty variantu v nějakém z projektů, kde Skautis používám(e), než budu schopen domyslet nějaký ergonomický balíček.