Skip to content
This repository has been archived by the owner on Feb 2, 2022. It is now read-only.

Use custom interval configuration #194

Closed

Conversation

mmachatschek
Copy link
Contributor

No description provided.

@sandervanhooft
Copy link
Collaborator

Thanks for taking a shot at this @mmachatschek , much appreciated.

You're now essentially combining the behaviour of three Interval generators in one. I think it would be better to split them up in three simple classes, all implementing the same Interval(Generator)Contract.

Then in the plan config, you could swap the generator out depending on your use case. The current interval config key can be used to pass in parameter values. As you suggested, either a string or an array.

What do you think?

@mmachatschek
Copy link
Contributor Author

@sandervanhooft sounds great! I'll update the PR and bump you as soon as you can have another look!

@mmachatschek
Copy link
Contributor Author

mmachatschek commented Apr 15, 2020

@sandervanhooft the PR now contains extendable Interval::class's

@mmachatschek mmachatschek changed the title Use interval configuration array Use custom interval configuration Apr 15, 2020
@mmachatschek
Copy link
Contributor Author

mmachatschek commented Apr 15, 2020

One more issue appeared upon going threw this.
What should happen when you reset the cycle? With a fixed interval now you will have problems with the first cycle as its reset to the day of creation of the subscription.

See Subscription::restartCycleWithModifications()

@sandervanhooft
Copy link
Collaborator

Subscription::restartCycleWithModifications() is resetting to the start of the cycle cycle_started_at, not the start of the subscription created_at.

@hadjedjvincent
Copy link

@mmachatschek you have a typo on IntervalConfigrationInvalidException class name, should be IntervalConfigurationInvalidException 😄

@mmachatschek
Copy link
Contributor Author

@sandervanhooft I'm closing this for now due to lack of time. If anyone else wants to pick up parts from this PR, feel free!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants