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.

Sign up for GitHub

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

Jump to bottom

fix!: Migrate jobs away from deprecated interfaces #43387

Merged
merged 4 commits into from
Feb 8, 2024
Merged

fix!: Migrate jobs away from deprecated interfaces #43387

merged 4 commits into from
Feb 8, 2024

Conversation

come-nc
Copy link
Contributor

@come-nc come-nc commented Feb 6, 2024 •
edited
Loading

Summary

Another nail in the coffin of ILogger.
See #32127

Checklist

@come-nc come-nc added the 2. developing Work in progress label Feb 6, 2024
@come-nc come-nc self-assigned this Feb 6, 2024
@come-nc come-nc mentioned this pull request Feb 6, 2024
Open
24 tasks
@come-nc come-nc force-pushed the fix/migrate-away-from-ilogger-in-jobs branch from d1a906f to 745448a Compare February 6, 2024 13:05
@come-nc come-nc added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Feb 6, 2024
@come-nc come-nc added this to the Nextcloud 29 milestone Feb 6, 2024
@ChristophWurst
Copy link
Member

  • Documentation (manuals or wiki) has been updated or is not required

This does need an entry in the upgrade docs.

@ChristophWurst ChristophWurst added the pending documentation This pull request needs an associated documentation update label Feb 6, 2024
@come-nc come-nc changed the title chore: Migrate jobs away from deprecated interfaces fix! Migrate jobs away from deprecated interfaces Feb 6, 2024
@come-nc come-nc changed the title fix! Migrate jobs away from deprecated interfaces fix!: Migrate jobs away from deprecated interfaces Feb 6, 2024
@come-nc come-nc force-pushed the fix/migrate-away-from-ilogger-in-jobs branch from 745448a to caa1a02 Compare February 6, 2024 16:28
nickvergessen
nickvergessen approved these changes Feb 6, 2024
View reviewed changes
apps/dav/lib/BackgroundJob/RefreshWebcalJob.php
}

/**
* this function is called at most every hour
*
* @inheritdoc
*/
public function execute(IJobList $jobList, ILogger $logger = null) {
public function start(IJobList $jobList): void {
Copy link
Member

Choose a reason for hiding this comment

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

🙈

@come-nc
fix!: Migrate jobs away from deprecated interfaces
1667738 
BREAKING CHANGE: Removed ILogFactory::getCustomLogger deprecated method

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
chore: Migrate away from internal Job classes
65981f9 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
chore: Delete deprecated internal Job classes
e78e271 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
chore: Remove last uses of IJob::execute method
b105603 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc come-nc force-pushed the fix/migrate-away-from-ilogger-in-jobs branch from caa1a02 to b105603 Compare February 8, 2024 09:28
@come-nc come-nc merged commit 8da9de9 into master Feb 8, 2024
141 checks passed
@come-nc come-nc deleted the fix/migrate-away-from-ilogger-in-jobs branch February 8, 2024 10:52
@come-nc come-nc mentioned this pull request Feb 8, 2024
Merged
@nickvergessen
Copy link
Member

above documentation PR looks like it's referencing the wrong thing.
This PR here should still be documented

@come-nc
Copy link
Contributor Author

come-nc commented Feb 8, 2024

above documentation PR looks like it's referencing the wrong thing. This PR here should still be documented

Can you elaborate on this?
The rest of the changes to lib/public are only aligning phpdoc with what was already true, the only other one is $logger parameter passed to execute is now ignored, is that what you’re referring as missing? The method was deprecated since 25 in IJob.

artonge pushed a commit that referenced this pull request Feb 8, 2024
@come-nc @artonge
Merge pull request #43387 from nextcloud/fix/migrate-away-from-ilogge…
303e2fe 
…r-in-jobs
@nickvergessen
Copy link
Member

Can you elaborate on this?

Let me rephrase then:

Even though OC\BackgroundJob\… were private, it was the (defacto) standard for quite some time.
I think it's worth to put a one-liner into https://docs.nextcloud.com/server/latest/developer_manual/app_publishing_maintenance/app_upgrade_guide/upgrade_to_29.html#id4 for it.

@come-nc come-nc mentioned this pull request Feb 20, 2024
Merged
@come-nc come-nc removed the pending documentation This pull request needs an associated documentation update label Feb 20, 2024
@blizzz blizzz mentioned this pull request Mar 5, 2024
Merged
@come-nc come-nc mentioned this pull request Sep 18, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChristophWurst ChristophWurst ChristophWurst approved these changes

@nickvergessen nickvergessen nickvergessen approved these changes

@blizzz blizzz Awaiting requested review from blizzz

Assignees

@come-nc come-nc

Labels
3. to review Waiting for reviews technical debt
Projects
None yet
Milestone
Nextcloud 29
Development

Successfully merging this pull request may close these issues.
3 participants
@come-nc @ChristophWurst @nickvergessen