Suggested improvements

[MartinMystikJonas]

I want to use this library for our SmartEmailing integration but I stumbled upon few issues. I can create PRs for them I just wanted to know if this package is still maintained and if suggested PRs yould be welcomed.

My suggestions:

• [x] Fix PHP8.1 support (incompatible method signatures, update to PHPUnit 8 or 9, some minor tweaks)
• [x] Updating CustomRequest so it deserializes data to data property if possible
• [x] Improve developer experience (add PHPStan, codign standard and Makefile to run them)
• [x] Create GitHub actions to run tests/PHPStan and coding standard checks
• [x] Fix Contactlist endpoint - currently only way to read data is to parse it directly from json
• [x] Add few new endpoints I am missing

[pionl]

Hi @MartinMystikJonas i personally do not use this package anymore, but PRs are welcomed (and sometimes their are coming ❤️)

As discussed in #23, PHP 8 would probably should be minimum. Is it ok for you?

I agree with everything you have mentioned :)

If possible this stack would be awesome:

• phpstan
• easy coding standard
• rector
• I have reusable workflow to run above (code coverage is saved to gist and badge can be used).
• These configs are currently use (needs some tweaking - I hate space between constants`).
• Makefile - I think it is not needed because we have only composer (i use composer check to run everything).

Of course you are free to use what you advice / use. Don't want to push something on you :)

Thank you! Msg me if you need anything from me.

[MartinMystikJonas]

I would need it on one legacy project with PHP 7.4 until I would have time to upgrade it to PHP 8. So for now I would keep PHP 7.4 compatibility.

[MartinMystikJonas]

I prepared two PRs 1) #25 For PHP8 compatibility 2) #26 Draft of developer tools. I need to check if GitHUb workflow works - can you enable it an trigger for this PR?

I also have locally fixeds of PHPStan errors, coding standard and autoamtic rector upgrade but I need to check it all first because Rector did some weird changes.

[MartinMystikJonas]

Jak teď ještě procházím kód tak se zdá, že neřídí nějakou konvenci v pojmenování/umístění Model, Request, Response, Endpoint.

Asi by bylo dobré to sjednotit, ale bude to BC break.

Osobně bych asi sjednotil takhle:

• Request
  • Foo
    • FooEndpoint.php
    • Bar (akce endpointu)
    • FooBarRequest.php
    • FooBarResponse.php
    • Baz (akce endpointu)
    • FooBazRequest.php
    • FooBazResponse.php

Otázka je kam s modelovými třídami a holdery. Teď jsou někdy ve složce Foo, někdy v podsložce Model a některé jsou dohromady ve složce \Request\Send. Osobně bych je vzhledem k tomu, že dost modelů budou sdílet různé endpointy to dal všechno do top namespace \Model.

[pionl]

Souhlasím, takto se to bude dobře hledat, skvělý postřeh :)

Jenom bych dal místo složky Request -> Requests, protože to bude obsahovat N-počet requestů.

[MartinMystikJonas]

No pokud už bysme přejmenovávali i tenhle namespace asi bych zvolil buď Api nebo Endopints

[pionl]

Dal bych endpoints a když už toho budeme takto šahat tak bych šel touto cestou :)

[MartinMystikJonas]

Jeste kdyz to ted predelavam tak sem narazil na to ze se pouzivaji 3 ruzne pristipy k metodam v Api/endpointech coz je trochu matouci.

1) ->something() rovnou posila request 2) ->something() vraci Request ktery jde upravit a pak se musi poslat pres ->send() 3) ->something() rovnou posila a ->somethingRequest() vraci Request

Byl bych pro to to sjednotit i za cenu BC breaks a to bud na verzi 3 a nebo to otocit vyjiz z verze 2 a mit

4) ->something() co vraci Request jako ve verzi 2 a pridat vzdycky jeste ->sendSomething() jako shortcut co to rovnou posle.

Kterou verzi bys preferoval?

[pionl]

Osobně v nových "API" SDKs to dělám tak, že mám funkce users()->create() -> vrací response.

Takže bych spíš se zamyslel, potřebujeme pracovat s request objektem?

Namátkově jsem vybral třídu co jsem dělal já a tam mám variantu 1.

Takže já spíš tíhnu 1. Co ty?

[MartinMystikJonas]

U slozitejsich requestu byva rada metod co upravuji/doplnuji obsah requestu pred odeslanim. Takze muzeme dat verzi 3 kde je ta moznost naplnit/upravit request pokud je potreba ale vychozi bude rovnou poslani

[MartinMystikJonas]

S tim ze u reqiest ktere bez naplneni nemaji smysl (treba import) by byla jen verze importRequest() a u requestu kde neni sance parametrizovat (treba ping) by byla jen verze ping() co rovnou odesila.

[pionl]

Ok :)

[MartinMystikJonas]

Ok překopu to tak. A ještě jenda otázka nemám rovnou sjednotit i názvy metod? Někde se pro načtení seznamu položek používá lists nekde search. Někde se pro načtení konkrétní položky používá get někde exists apod

[pionl]

Pokud na to máš chut a prostor tak bych to rovnou udělal! :)

[MartinMystikJonas]

Navrhoval bych to u endpointů, které umožňují CRUD takhle:

• get($id): ?Model - vrací jednu položku modelu podle ID
• list($page = null, $limit = null): array - vrací pole položek modelu
• searchRequest($page = null, $limit = null): Request - vrací request, který jde doplnit a pak odeslat přes send()
• create($data): void - vytvoří novou položku
• createRequest($data) - vytvoří request na přidání položky, který jde doplnit a pak odeslat přes send()

[pionl]

To vypadá dobře :) Souhlas

[MartinMystikJonas]

Ready v #28 zkus to pls zkouknout jestli je to ok. Pokud to bude ok jen doplním readme a může se to mergnout. Pak se vrhnu na ty endpointy co mi aktuálně chybí

[pionl]

Ahoj, díky mrknu na to

[MartinMystikJonas]

Začal sme s tím doplňováním typů a už to mám skoro hotové. Jen jsme teď anrazil na jednu další nekonzistenci. Modelové třídy někdy používají public properties, někdy public+settery a někde private+gettery+settery. To by asi taky bylo fajn sjednotit, ale bude to další BC break.

IMHO bych asi volil řešení s gettery+settery.

Případně bysme mohli v modelu implemenotvat i get set magic metody a předávat to na gettery+settery + doplnit @property anotace. Ale to je IMHO zbytečné.

[pionl]

IMHO bych asi volil řešení s gettery+settery.

Souhlas, jsem spíš více pro staticky safe než annotace :)