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

Include task scheduling in core #2704

Merged
merged 2 commits into from
Mar 19, 2021
Merged

Include task scheduling in core #2704

merged 2 commits into from
Mar 19, 2021

Conversation

askvortsov1
Copy link
Sponsor Member

@askvortsov1 askvortsov1 commented Mar 17, 2021

Fixes #2281

Changes proposed in this pull request:
Add a schedule method to the Console extender.

Reviewers should focus on:

  • Should we add methods for each way to schedule? There's a LOT of options, so I think what I have here is simpler. On the other hand it's slightly less BC compatible, in case laravel decides to break something on its side. But I still think its way too many methods, and figuring out how to chain them is complicated.
  • Turns out we need the helpers we literally just removed for this.... Given this it might make sense to add them back, but still discourage use just like we do now. Should we do this in the same PR?

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).

composer.json Outdated Show resolved Hide resolved
@askvortsov1
Copy link
Sponsor Member Author

When merging, if we decide to keep the helper changes done here, I'll split it into 2 commits:

  1. the helper stuff
  2. everything else

Copy link
Member

@imorland imorland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checks out fine locally

src/Console/Schedule.php Outdated Show resolved Hide resolved
composer.json Show resolved Hide resolved
@askvortsov1 askvortsov1 requested a review from SychO9 March 19, 2021 12:34
Copy link
Member

@SychO9 SychO9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add methods for each way to schedule? There's a LOT of options, so I think what I have here is simpler. On the other hand it's slightly less BC compatible, in case laravel decides to break something on its side. But I still think its way too many methods, and figuring out how to chain them is complicated.

We could make an adapter that reroutes method calls to the actual Event class, that way if laravel breaks something we'll have a BC layer, that said typehinting would be a problem, we could use a phpDoc block for that, but maintaining that can easily become a pain.

@askvortsov1
Copy link
Sponsor Member Author

Should we add methods for each way to schedule? There's a LOT of options, so I think what I have here is simpler. On the other hand it's slightly less BC compatible, in case laravel decides to break something on its side. But I still think its way too many methods, and figuring out how to chain them is complicated.

We could make an adapter that reroutes method calls to the actual Event class, that way if laravel breaks something we'll have a BC layer, that said typehinting would be a problem, we could use a phpDoc block for that, but maintaining that can easily become a pain.

I think it's safe to assume that if Laravel breaks this, they've probably broken other things, meaning that we'd only be including that upgrade in a major release.

@askvortsov1 askvortsov1 force-pushed the as/task-scheduling branch 2 times, most recently from f097c5d to 3addcfb Compare March 19, 2021 21:55
@askvortsov1 askvortsov1 merged commit 8dd57ff into master Mar 19, 2021
@askvortsov1 askvortsov1 deleted the as/task-scheduling branch March 19, 2021 22:01
}
}

class FakeApp
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Multiple class declarations in one file are really not done. It makes discoverability extremely hard. Either:

  • move that class into its own file
  • or use an anonymous class

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I keep forgetting anonymous classes exist. This is the perfect use case for one. I'll open a PR to clean this up, thanks!

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.

Integrate task scheduling into core
5 participants