Skip to content
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

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

Open
fmasa opened this issue May 19, 2020 · 13 comments
Open

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

fmasa opened this issue May 19, 2020 · 13 comments

Comments

@fmasa
Copy link
Member

fmasa commented May 19, 2020

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

  • Bylo by možné zpřístupnit async API (což jestli dobře chápu byla hlavní motivace pro BatchRequest SOAP #29)
  • Jednodušší testování - ve Skautském Hospodaření mockujeme Soap komunikaci přes PHP-VCR, pro Guzzle je mnohem jednodušší testy psát (jsou tam přímo utility middlewares jako HistoryMiddleware apod). Kromě toho některé části ani nejdou namockovat (SoapClient při vytvoření instance stahuje wsdl soubor, což není možné nijak přetížit).
  • V skautis/nette nebudeme muset mít vlastní panel pro Tracy - bude fungovat jakýkoliv panel pro Guzzle (např. https://github.com/contributte/guzzlette)
    Nemám problém to připravit a poslat PR, ale zajímal by mě ohlas na podobnou změnu.
@JindrichPilar
Copy link
Member

JindrichPilar commented May 24, 2020

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
Copy link
Member Author

fmasa commented May 24, 2020

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
Copy link
Member Author

fmasa commented May 24, 2020

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
Copy link
Member Author

fmasa commented May 24, 2020

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
Copy link
Member

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
Copy link
Member Author

fmasa commented May 27, 2020

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
Copy link
Member

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
Copy link
Member Author

fmasa commented Jun 17, 2020

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:

  • Guzzle nemusí být nainstalovaný jako uživatelova root dependency - může být jako závislost jiného balíčku - v tom případě bychom určitě neměli automaticky použít Guzzle (pokud uživatel následně tu závislost, které požaduje Guzzle, odebere, tak se mu změní chování.
  • Knihovna by musela mít konflikt v composer.json s jinými verzemi Guzzle, i když by na nich přímo nezávisela. Jinak by uživatel mohl nainstalovat nekompatibilní verzi Guzzle.*

*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
Copy link
Member

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

  • V teto knihovne vytvorime interface pro promise, tak aby kopiroval Guzzle promise.
  • Upravime WebServiceInterface, aby mel metodu asyncCall s temer stejnou signature jako call, ale bude vracet promise. Do PHPDocu dame varovani ze zalezi na pouzite implementaci a muze byt ve skutecnosti sync.
  • Upravime WebService, aby obsahovala asyncCall, s tim ze bude fungovat synchrone.
  • Vytvorime novy balicek skaut/skautis-async.
  • Do suggest dame skaut/skautis-async.
  • Pridame dokumentaci.

@marekdedic
Copy link
Contributor

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
Copy link
Member

JindrichPilar commented Jun 21, 2020

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
Copy link
Contributor

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
Copy link
Member Author

fmasa commented Jun 23, 2020

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants