Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[Scheduler] Fix#[AsCronTask] not passing arguments to command#60741

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

@jan-pintr
Copy link
Contributor

QA
Branch?7.3
Bug fix?yes
New feature?no
Deprecations?no
Issues-
LicenseMIT

This MR fixes passingAsCronTask arguments toCommand. Reproduced can be by following command. It wont receive--all option or any other argument specified inAsCronTask when executed by Scheduler.

#[AsCommand(name: 'debug:as-cron-task')]#[AsCronTask('* * * * *', arguments: '--all')]class SampleCommand extends Command{    protected function configure(): void    {        $this->addOption('all', null, InputOption::VALUE_NONE);    }    protected function execute(InputInterface $input, OutputInterface $output): int    {        dump($input->getOption('all'));        return Command::SUCCESS;    }}

Fix description: When command name was found inAsCommand attribute then attributes were not appended to message definition due to missing brackets.

imba28, michaljusiega, and gassan reacted with thumbs up emoji
Copy link
Member

@kbondkbond left a comment

Choose a reason for hiding this comment

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

Thanks for finding/fixing this! Can you target 6.4?

Do you think you'd be up for adding some testshere to prevent a regression?

@jan-pintrjan-pintr changed the base branch from7.3 to6.4June 9, 2025 15:54
@jan-pintrjan-pintr changed the base branch from6.4 to7.4June 9, 2025 15:54
@jan-pintrjan-pintr changed the base branch from7.4 to7.3June 9, 2025 17:24
@jan-pintr
Copy link
ContributorAuthor

Test added. But branch was reverted to 7.3, because the problem was introduced in 7.3 (here#59711) Is it ok?

valtzu reacted with thumbs up emoji

@kbond
Copy link
Member

because the problem was introduced in 7.3

Oh, my bad - so the problem doesn't exist < 7.3?

@jan-pintr
Copy link
ContributorAuthor

Yes, it did not exist in <7.3

@lyrixxlyrixx removed their request for reviewJune 10, 2025 08:28
Copy link
Member

@kbondkbond left a comment

Choose a reason for hiding this comment

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

Code looks good! I'm not 100% sure of the issue with the one CI failure.

@jan-pintr
Copy link
ContributorAuthor

Not sure either. Before test fails there is logInstalling symfony/scheduler (7.4.x-dev ac9f564): Extracting archive.

I suppose that the reason is that thehigh-deps is runned against7.4.x-dev in which it is not fixed yet? Could that be the reason?

@kbond
Copy link
Member

kbond commentedJun 10, 2025
edited
Loading

Could that be the reason?

Yeah, that's what I'm thinkingbut I would have expected this issue with "low-deps" (oh, I forgot this wasn't an issue in 6.4 so that makes sense).

It should be fixed once merged though.

@OskarStarkOskarStark changed the title[Scheduler] Fix AsCronTask not passing arguments to command[Scheduler] Fix#[AsCronTask] not passing arguments to commandJun 20, 2025
@jan-pintr
Copy link
ContributorAuthor

Suggestion applied, thank you.

@nicolas-grekasnicolas-grekasforce-pushed thefix-ascrontask-no-passing-arguments branch fromcf6c25d tob57a815CompareJuly 9, 2025 09:46
@nicolas-grekas
Copy link
Member

Thank you@jan-pintr.

@nicolas-grekasnicolas-grekas merged commit7095ee9 intosymfony:7.3Jul 9, 2025
9 of 11 checks passed
@fabpotfabpot mentioned this pull requestJul 31, 2025
@johndodev
Copy link

Hello,

thanks,

Idk if this is a real problem or something supported, but as there is no nullsafe check on $tagAttributes['arguments']here, this is breaking a tag on my use case, which is a "scheduler.task" tag added in a LoadExtension bundle I'm currently developping.

I'm notifying because every others attributes seems optionnals :

$myService->tag('scheduler.task', ['frequency' =>15,'schedule' =>$config['schedule'],'trigger' =>'every','arguments' =>null,// <- had to add this line to avoid a Warning: Undefined array key "arguments"]);

@jan-pintr
Copy link
ContributorAuthor

@johndodev Please look at#61307.

Even at leasttrigger and thenfrequency (fortrigger: every) orexpression (fortrigger: cron) parameters are also mandatory then requiring specifyarguments => null seems not intended.

johndodev reacted with thumbs up emoji

nicolas-grekas added a commit that referenced this pull requestAug 5, 2025
… (Jan Pintr)This PR was squashed before being merged into the 7.3 branch.Discussion----------[Scheduler] Fix `scheduler.task` tag arguments optionality| Q             | A| ------------- | ---| Branch?       | 7.3| Bug fix?      | yes| New feature?  | no| Deprecations? | no| Issues        | -| License       | MITAccording to [this comment](#60741 (comment)) there is a change in behaviour after#60741 which causes requiring specify `arguments` parameter when tagging `scheduler.task` manually. This pull request makes `arguments` parameter back optional.```$myService->tag('scheduler.task', [    'trigger' => 'every',    'frequency' => 15,    'arguments' => null, // <- had to add this line to avoid a Warning: Undefined array key "arguments"]);```Commits-------ad08041 [Scheduler] Fix `scheduler.task` tag arguments optionality
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@kbondkbondkbond approved these changes

@OskarStarkOskarStarkOskarStark approved these changes

@ycerutoycerutoAwaiting requested review from yceruto

@chalasrchalasrAwaiting requested review from chalasr

@dunglasdunglasAwaiting requested review from dunglas

@jderussejderusseAwaiting requested review from jderusse

@xabbuhxabbuhAwaiting requested review from xabbuh

@fabpotfabpotAwaiting requested review from fabpot

Assignees

No one assigned

Projects

None yet

Milestone

7.3

Development

Successfully merging this pull request may close these issues.

7 participants

@jan-pintr@kbond@nicolas-grekas@johndodev@fabpot@OskarStark@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp