Skip to content

Conversation

tomasfejfar
Copy link
Contributor

In 7.0 run() is no longer ran from index.php, so it needs to be a default value if it is to work with empty config {}.

@tomasfejfar tomasfejfar self-assigned this Mar 18, 2019
@tomasfejfar tomasfejfar requested a review from odinuv March 18, 2019 13:11
@tomasfejfar
Copy link
Contributor Author

@odin, tohle jsem objevil, když jsem ještě upravoval ten PR do generátoru. Určitě by se to nemělo chovat jako teď (tj. Keboola\Component\Exception\BaseComponentException:Unknown sync action "", method does not exist in class). Buďto by to mělo vykysat na exception, že je to neplatný config nebo by to mělo mít default run.

@odinuv
Copy link
Member

odinuv commented Mar 18, 2019

seriozne nevim, mam z toho ambivalentni pocity - na jednu stranu je to prijemny pro testy, na druhou stranu je to nerealisticka konfigurace, tak proc se s ni zaobirat?

@tomasfejfar tomasfejfar force-pushed the tf-make-run-optional-as-before branch from 33ce3db to ea20ed1 Compare March 18, 2019 13:15
@tomasfejfar
Copy link
Contributor Author

Tak v tom případě by to mělo padat pokud ten "action" chybí. Ale teď že to vrátí '' je imho blbě a bylo to OK dřív tím,ž e se s tím nikde nepracovalo.

@odinuv
Copy link
Member

odinuv commented Mar 18, 2019

no tak pak by tam nemelo byt return $this->getValue(['action'], ''); ale jen return $this->getValue(['action']);

@tomasfejfar
Copy link
Contributor Author

tomasfejfar commented Mar 18, 2019

Jo, přesně tak. Ten default je tam historicky už od začátku prázdný string.

@odinuv
Copy link
Member

odinuv commented Mar 18, 2019

ale klidne se pojdme zeptat jeste nekoho jinyho jestli jit cestou a nebo b, protoze ja si fakt nejsem jistej

@tomasfejfar
Copy link
Contributor Author

@tomasfejfar tomasfejfar force-pushed the tf-make-run-optional-as-before branch from ea20ed1 to efc55a4 Compare March 18, 2019 16:23
@odinuv
Copy link
Member

odinuv commented Mar 18, 2019

btw, vsim jsem si, ze https://github.com/keboola/php-component#migration-from-version-6-to-version-7 tady zustal run misto execute

@tomasfejfar
Copy link
Contributor Author

Jo, přilepím fix k tomu, díky 👍

@tomasfejfar tomasfejfar merged commit 5a068b1 into master Mar 19, 2019
@tomasfejfar tomasfejfar deleted the tf-make-run-optional-as-before branch March 19, 2019 13:39
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

Successfully merging this pull request may close these issues.

2 participants