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

Adding Registering alias feature to the package #132

Merged
merged 5 commits into from
Jan 14, 2022
Merged

Adding Registering alias feature to the package #132

merged 5 commits into from
Jan 14, 2022

Conversation

nyamsprod
Copy link

This PR is inspired by my work on bakame/cron to resolve #64

here's the proposed public API:

<?php

use Cron\CronExpression;

if (!CronExpression::supportsAlias('@every')) {
    CronExpression::registerAlias('@every', '* * * * *');
}

CronExpression::getAliases();
// returns
// array (
//   '@yearly' => '0 0 1 1 *',
//   '@annually' => '0 0 1 1 *',
//   '@monthly' => '0 0 1 * *',
//   '@weekly' => '0 0 * * 0',
//   '@daily' => '0 0 * * *',
//   '@midnight' => '0 0 * * *',
//   '@hourly' => '0 * * * *',
//   '@every' => '* * * * *',
// )
CronExpression::supportsAlias('@foobar'); //return false
CronExpression::supportsAlias('@daily');  //return true
CronExpression::supportsAlias('@every');  //return true

$cron = new CronExpression('@every');  // works!
$cron->getExpression(); // return * * * * *

CronExpression::unregisterAlias('@every'); // return true
CronExpression::unregisterAlias('@every'); // return false 
                                           // because on the second time the alias is already removed!

CronExpression::supportsAlias('@every');   //return false

new CronExpression('@every');  //throws InvalidArgumentException unknown or unsupported expression
CronExpression::unregisterAlias('@daily');  //throws LogicException exception

Comments and remarks are welcomed.
PS: while running the test as is on PHP8.1 some tests were failing prior to my addition via this PR.

@dragonmantank
Copy link
Owner

Overall this looks good, except the tests are failing. Looks like it's expecting CronExpression::aliases() but it was implemented as CronExpression::getAliases(). That should clear that up.

Can you also rebase/update to the latest master? That should clear up the conflict from some new tests that were added.

Thanks!

@nyamsprod
Copy link
Author

PR rebased to master and fix method call in test.

@dragonmantank dragonmantank merged commit d10f969 into dragonmantank:master Jan 14, 2022
@dragonmantank
Copy link
Owner

Thanks!

@nyamsprod nyamsprod deleted the feature/handle-aliases branch March 12, 2022 19:24
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.

Add ability to allow user-supplied shortcuts
2 participants