keboola / php-component

General library for PHP applications running in Keboola Connection environment
MIT License
0 stars 1 forks source link

No manifest should behave like empty manifest #35

Closed odinuv closed 6 years ago

odinuv commented 6 years ago

ErrorException : file_get_contents(C:\Users\Odin\AppData\Local\Temp/wr-storage/run-5afc5eca2e0003.61380871/in/tables/some-table-1.manifest): failed to open stream: No such file or directory D:\Dropbox\wwwroot\wr-storage\vendor\keboola\php-component\src\Manifest\ManifestManager.php:97

tomasfejfar commented 6 years ago

Fakt myslíš, že je to bug? Souhlasím, že by to mělo kontrolovat, že ten soubor existuje, ale nejsem si úplně jistý, že chceš, aby to potichu ignorovalo, že neexistuje. Mohly by z toho být těžko odhalitelné bugy. Např. když se upíšeš v názvu manifestu, tak chceš, aby ti to řeklo a ne aby to tiše "jakože načetlo", ne?

odinuv commented 6 years ago

hmm, no trosku jsi me nalomil, ale manifest je vec v zasade nepovinna - my ji generujeme vzdycky, ale kdyz tam neni, tak to nemusi byt chyba. Myslim, ze to, ze se prepises v nazvu manifestu by ti meli odhalit testy komponenty

tomasfejfar commented 6 years ago

Co kdyby to načetlo jako empty array, ale vypsalo warning/notice do logu?

odinuv commented 6 years ago

ja, fakt nevim no, @ondrejhlavacek ?

ondrejhlavacek commented 6 years ago

Pokud se upises v názvu manifestu, tak by to mělo zařvat, že neexistuje datovej soubor.

tomasfejfar commented 6 years ago

Takže podle tebe by načítání manifestu mělo kontrolovat, jestli existuje datový soubor k tomu manifestu a pokud ne, tak failnout?

Takhle?

$manifestWithTypo = 'fileanme1';
file_exists($manifestWithTypo); //false
file_exists($manifestWithTypo . '.manifest'); // false
getManifest() // Exception('Data file "fileanme1" for manifest does not exists');
$manifestMissing = 'filename1';
file_exists($manifestMissing); // true
file_exists($manifestMissing. '.manifest'); // false
getManifest() // []
ondrejhlavacek commented 6 years ago

jj, přesně, mělo by se to chovat konzistentně s output mappingem

https://github.com/keboola/output-mapping/blob/15c227217e0e9eebf875620c65a7b4f2a580d650/Writer/Writer.php#L243-L247

odinuv commented 6 years ago

no to je zbytecny delat tuhle kontrolu v php-component ne? od toho to zachytava docker runner

ondrejhlavacek commented 6 years ago

jo, pravda, nějak jsem se vytrhl z kontextu

tomasfejfar commented 6 years ago

Tak to jste mi moc nepomohli :) Ať se někam posuneme, tak to pojďme zvotovat:

Načtení neexistujícího manifestu by mělo 👍 vrátit [] 👎 vyhodit exception 😕 vrátit [] a do logu poslat notice

ondrejhlavacek commented 6 years ago

Manifest není povinnej, takže notice není imho potřeba vyhazovat.