Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make bundle a bit more extendable. #310

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

zilionis
Copy link

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

@coveralls
Copy link

coveralls commented Dec 28, 2017

Coverage Status

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

@coveralls
Copy link

coveralls commented Dec 28, 2017

Coverage Status

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

@craue
Copy link
Owner

craue commented Jan 2, 2018

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
Copy link
Author

zilionis commented Jan 2, 2018

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.

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.

3 participants