CraueFormFlowBundle icon indicating copy to clipboard operation
CraueFormFlowBundle copied to clipboard

Make bundle a bit more extendable.

Open zilionis opened this issue 7 years ago • 4 comments

At this time is impossible to extend some methods, because it uses private methods where is really not needed.

zilionis avatar Dec 28 '17 10:12 zilionis

Coverage Status

Coverage decreased (-0.3%) to 99.454% when pulling 973195a5c38361dffc7f453511ad80877b227357 on zilionis:master into e933192808203428cea49ae1d9080018d1d320cd on craue:master.

coveralls avatar Dec 28 '17 10:12 coveralls

Coverage Status

Coverage decreased (-0.2%) to 99.509% when pulling 0f96df2ce749d224732ee8858b1e96524607062c on zilionis:master into e933192808203428cea49ae1d9080018d1d320cd on craue:master.

coveralls avatar Dec 28 '17 11:12 coveralls

I'm against exposing every single attribute as it makes it harder for me to change the code while keeping BC.

The code extracted to isNeedToResetFlow does more than determining the return value,

So the only changes I'd merge are the new methods isNewInstance and isExpired, but please move them above the methods meant for BC.

craue avatar Jan 02 '18 13:01 craue

From one side I agree, what is not always good expose all attributes. But we have another side.

Looks like some methods doing to much. we can split them to smaller ones and use getters/setters for them. So dev can extend some methods, add additional bussiness logic and will not stuck because one argument is private.

For example for some case i need to get instanceId at the beging of flow. And it should not change. (It's important to have correct one at the beging, because it changes max 3 times: flow_xxx, uniqBlobToString and again uniqBlobToString on expire)

example you can do this:

$flow->bind(new FormRequest()); // for new instance
$flow->bind($formRepo->getByInstance($flow->getInstanceId())); // if we have some steps in db already 

both cases binds formData, thats we need. At current version is impossible to fix this. because formData is private :(

Another small example. Imagine you want replace instanceId to another format UUID :) is impossible to do, because $this->newInstance is private :)

....

yeah isNeedToResetFlow is doing to much (like determineInstanceId with newInstance = true), i just moved some code from bindFlow method. Just i havn't enough time properly to refactor this method.

zilionis avatar Jan 02 '18 14:01 zilionis