craue / CraueFormFlowBundle

Multi-step forms for your Symfony project.
MIT License
736 stars 118 forks source link

Make bundle a bit more extendable. #310

Open zilionis opened 6 years ago

zilionis commented 6 years ago

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

coveralls commented 6 years ago

Coverage Status

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

coveralls commented 6 years ago

Coverage Status

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

craue commented 6 years ago

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.

zilionis commented 6 years ago

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.