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

Deprecated ILogger from IJob #33047

Merged
merged 3 commits into from
Aug 23, 2022
Merged

Deprecated ILogger from IJob #33047

merged 3 commits into from
Aug 23, 2022

Conversation

CarlSchwan
Copy link
Member

@CarlSchwan CarlSchwan commented Jun 28, 2022 •
edited
Loading

Since the ILogger will be soon removed we need to ensure that nothing in
the public API use it.

  • Add start to replace execute but without ILogger
  • execute still exists but only call start internally
  • Use more OCP interface for background job
  • Fix a lot of warning in tests related to background jobs

@CarlSchwan CarlSchwan added the 3. to review Waiting for reviews label Jun 28, 2022
@CarlSchwan CarlSchwan added this to the Nextcloud 25 milestone Jun 28, 2022
@CarlSchwan CarlSchwan requested a review from come-nc June 28, 2022 10:10
@CarlSchwan CarlSchwan self-assigned this Jun 28, 2022
github-advanced-security[bot]
github-advanced-security bot found potential problems Jun 28, 2022
View reviewed changes
apps/federation/lib/BackgroundJob/GetSharedSecret.php Fixed Show fixed Hide fixed
apps/user_ldap/lib/Jobs/Sync.php Fixed Show fixed Hide fixed
apps/federation/lib/BackgroundJob/GetSharedSecret.php Fixed Show fixed Hide fixed
apps/federation/lib/BackgroundJob/GetSharedSecret.php
@@ -213,22 +175,22 @@
);
} else {
$this->logger->error(
'remote server "' . $target . '"" does not return a valid shared secret. Received data: ' . $body,
['app' => 'federation']
'remote server "' . $target . '"" does not return a valid shared secret. Received data: ' . $body,

Check notice

Code scanning / Psalm

PossiblyInvalidOperand

Cannot concatenate with a resource|string
@come-nc
Copy link
Contributor

come-nc commented Jun 28, 2022

❤️

@CarlSchwan CarlSchwan force-pushed the fix/ijob-logger-deprecated branch 3 times, most recently from 6721a06 to b700c8b Compare June 29, 2022 12:40
@ChristophWurst
Copy link
Member

Since the ILogger will be soon

It will? 🎉

ChristophWurst
ChristophWurst approved these changes Jun 29, 2022
View reviewed changes
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Lots of good cleanup

Personally I would prefer if the PR only touched the logger aspects if that is what title and subscription say. E.g. background job OC to OCP kinda feels sneaked in.

But I understand it's easier this way, so 👍 and 🤞

@CarlSchwan
Copy link
Member Author

Since the ILogger will be soon

It will? tada

still one year to wait since we need to wait 3 years after it has been deprecated, but since execute(...., ILogger) was for some app the only reasons why ILogger was still used, this should make it easier to get rid of it as soon as the 3 years are over.

@CarlSchwan CarlSchwan force-pushed the fix/ijob-logger-deprecated branch 3 times, most recently from af33a00 to 3c7280a Compare July 14, 2022 13:31
@github-actions
Copy link
Contributor

Possible performance regression detected

Show Output 
1 queries added

= /remote.php/dav/files/test
= /remote.php/dav/files/test/test.txt
= /remote.php/dav/files/test/many_files
= /remote.php/dav/files/test/new_file.txt
≠ /remote.php/dav/files/test/new_file.txt with 1 queries added
  + UPDATE "oc_preferences" SET "configvalue" = :dcValue1 WHERE ("userid" = :dcValue2) AND ("appid" = :dcValue3) AND ("configkey" = :dcValue4)

@CarlSchwan CarlSchwan force-pushed the fix/ijob-logger-deprecated branch 5 times, most recently from 6fa90dd to 06e1aa9 Compare July 14, 2022 15:34
@github-actions
Copy link
Contributor

Possible performance regression detected

Show Output 
2 queries added

= /remote.php/dav/files/test
= /remote.php/dav/files/test/test.txt
≠ /remote.php/dav/files/test/many_files with 1 queries added
  + UPDATE "oc_preferences" SET "configvalue" = :dcValue1 WHERE ("userid" = :dcValue2) AND ("appid" = :dcValue3) AND ("configkey" = :dcValue4)
≠ /remote.php/dav/files/test/new_file.txt with 1 queries added
  + UPDATE "oc_preferences" SET "configvalue" = :dcValue1 WHERE ("userid" = :dcValue2) AND ("appid" = :dcValue3) AND ("configkey" = :dcValue4)
= /remote.php/dav/files/test/new_file.txt

@come-nc
Copy link
Contributor

come-nc commented Jul 28, 2022

PHP Fatal error: Cannot use OCP\Files\SimpleFS\ISimpleFile as ISimpleFile because the name is already in use in /home/runner/work/server/server/tests/Core/Controller/AvatarControllerTest.php on line 42

@CarlSchwan CarlSchwan force-pushed the fix/ijob-logger-deprecated branch 2 times, most recently from 0fead00 to 5d02024 Compare July 29, 2022 09:37
@CarlSchwan CarlSchwan force-pushed the fix/ijob-logger-deprecated branch 2 times, most recently from b56a561 to f60c1a3 Compare July 29, 2022 15:06
@CarlSchwan
Deprecated ILogger from IJob
19a2d6f 
Since the ILogger will be soon removed we need to ensure that nothing in
the public api use it.

Signed-off-by: Carl Schwan <[email protected]>
@CarlSchwan
Port existing server code to new interface
48d9c4d 
Signed-off-by: Carl Schwan <[email protected]>
@CarlSchwan
Fix many tests and warnings
49334e4 
Signed-off-by: Carl Schwan <[email protected]>
This was referenced Aug 12, 2022
Merged
Merged
@CarlSchwan
Copy link
Member Author

ci failure unrelated

@CarlSchwan CarlSchwan merged commit b888c61 into master Aug 23, 2022
@CarlSchwan CarlSchwan deleted the fix/ijob-logger-deprecated branch August 23, 2022 14:55
@blizzz blizzz mentioned this pull request Aug 24, 2022
Merged
@joshtrichards joshtrichards mentioned this pull request Sep 2, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nickvergessen nickvergessen nickvergessen left review comments

@ChristophWurst ChristophWurst ChristophWurst approved these changes

@come-nc come-nc come-nc approved these changes

@kesselb kesselb Awaiting requested review from kesselb

@icewind1991 icewind1991 Awaiting requested review from icewind1991

Assignees

@CarlSchwan CarlSchwan

Labels
3. to review Waiting for reviews
Projects
None yet
Milestone
Nextcloud 25
Development

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