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

Cleanup cron.php method calls #43454

Merged
merged 6 commits into from
Feb 8, 2024
Merged

Cleanup cron.php method calls #43454

merged 6 commits into from
Feb 8, 2024

Conversation

come-nc
Copy link
Contributor

@come-nc come-nc commented Feb 8, 2024

Summary

Cleanup cron.php and bring it to the modern world. Added strict_types as well.

Checklist

@come-nc come-nc added the 3. to review Waiting for reviews label Feb 8, 2024
@come-nc come-nc added this to the Nextcloud 29 milestone Feb 8, 2024
@come-nc come-nc self-assigned this Feb 8, 2024
IConfig is a small wrapper around SystemConfig, no need to pull both of them

Signed-off-by: Côme Chilliet <[email protected]>
@come-nc come-nc requested review from nickvergessen, juliushaertl, a team, ArtificialOwl, icewind1991 and Fenn-CS and removed request for a team February 8, 2024 13:40
@susnux susnux added the bug label Feb 8, 2024
@susnux susnux merged commit cdf7840 into master Feb 8, 2024
141 checks passed
@susnux susnux deleted the fix/cleanup-cron-php branch February 8, 2024 15:21
@solracsf
Copy link
Member

solracsf commented Feb 8, 2024

Doesn't this need an adjustement?

/**
* Run the background job with the registered argument
*
* @param IJobList $jobList The job list that manages the state of this job
* @param ILogger|null $logger
* @since 7.0.0
* @deprecated since 25.0.0 Use start() instead. This method will be removed
* with the ILogger interface
*/
public function execute(IJobList $jobList, ILogger $logger = null);

@come-nc
Copy link
Contributor Author

come-nc commented Feb 12, 2024

Doesn't this need an adjustement?

/**
* Run the background job with the registered argument
*
* @param IJobList $jobList The job list that manages the state of this job
* @param ILogger|null $logger
* @since 7.0.0
* @deprecated since 25.0.0 Use start() instead. This method will be removed
* with the ILogger interface
*/
public function execute(IJobList $jobList, ILogger $logger = null);

No, why?
The method was already deprecated as you see in the lines you linked to.
It will be removed along with ILogger class, I do not think it’s useful to remove it before.

@solracsf
Copy link
Member

Because it espects ILogger and we're now passing LoggerInterface.

The method was already deprecated as you see in the lines you linked to.

Fine :)

@come-nc
Copy link
Contributor Author

come-nc commented Feb 12, 2024

Because it espects ILogger and we're now passing LoggerInterface.

No we’re not, where?
Psalm would complain about it if we did I think.

Also I tried to remove all calls to execute, if you find some please list them.

@solracsf
Copy link
Member

I've saw it somewhere in PSALM for another PR... 🤔
But if PSALM is happy now, i'm hapy too :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants