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

Clean up (many) deprecated functions #40083

Closed
wants to merge 1 commit into from

Conversation

summersab
Copy link
Contributor

@summersab summersab commented Aug 28, 2023

  • Resolves: # N/A for now

Summary

There are thousands of deprecated functions, constants, and classes in the NC core. Some of them are actually noted to be removed in previous releases, yet they still persist.

I am a strong believer in dogfooding your own code and APIs. Therefore, I took some time to clean up a good chunk of the deprecated code. Nothing in 3rdparty or apps has been changed.

Since this is a HUGE commit (and I may find more instances to add - suggestions welcome), I am leaving it as a draft for the moment.

TODO

  • ...

Checklist

Signed-off-by: Andrew Summers <18727110+summersab@users.noreply.github.com>
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

Psalm found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

@solracsf
Copy link
Member

IMO, you should not commit all these changes in a single commit but split in multiple commits. This will help get this reviewed correctly. 👍

@solracsf solracsf added the 2. developing Work in progress label Aug 29, 2023
@solracsf solracsf added this to the Nextcloud 29 milestone Aug 29, 2023
@summersab
Copy link
Contributor Author

IMO, you should not commit all these changes in a single commit but split in multiple commits. This will help get this reviewed correctly. 👍

That was a question I was going to have for one of the devs. I thought that perhaps this wouldn't be such a big deal since I only replaced functions that are basically drop-in replacements (i.e. the ones where the function call in OC\Server resolves to a single line, so the only thing that needs to be replaced is that line of code and adding a use statement to the top of the class).

There are only two exceptions to this:

  • Replacing OC\Server::getLogger() with OC\Server::get(Psr\Log\LoggerInterface::class): The OCP\ILogger class returned by getLogger() has a logException() method that isn't present in LoggerInterface, so I had to do some rewrites.
  • IQueryBuilder::execute() is replaced by IQueryBuilder::executeStatement() or IQueryBuilder::executeQuery(). The former returns an integer while the latter returns a result object, so making sure to use the right one is critical.

Besides those, they're really are just one-to-one replacements. However, if you think it's best, I can split things into separate commits.

Question. Should changes to ./apps be in their own individual PRs? I didn't want to touch those since I figured it would mean changing the app version to issue an update, etc.

@kesselb
Copy link
Contributor

kesselb commented Aug 29, 2023

Thanks for your pull request 👍

A good amount of our CI suite is broken 😞

Changing so many things in a single commit is not a good idea.
Noone is able to review 219 changed files properly.

We got a couple of refactoring pull requests recently.
They are usually scoped by function (e.g. replace strpos with str_contains) or component (e.g. modernize lib/Security).

Please use them as example: https://github.com/nextcloud/server/pulls?q=is%3Apr+sort%3Aupdated-desc+is%3Aclosed+refactor

@kesselb kesselb closed this Aug 29, 2023
@kesselb
Copy link
Contributor

kesselb commented Aug 29, 2023

Sorry, I didn't intend to close the pull request right away.
Hit the wrong button 🙈

@summersab
Copy link
Contributor Author

No worries, @kesselb. Since you recommend splitting this up, the PR should probably be closed, anyway.

@nickvergessen
Copy link
Member

@summersab Thanks for the willingness to help to improve the code quality.

Unfortunatly the way you did this now is also rather painful as well. You spamed 40+ PR and therefore blocked Drone CI from 3am until 4:30pm European time (that is when the first other PR received their results) with your refactorings (And since an hour ago when @solracsf approved the GitHub Actions run on most/all of them, GitHub Actions are mostly blocked for the rest of the day). Maybe you can focus on one PR after another, so rest of the Nextclouders is still able to work and get CI results during the same day.

Most of your PRs also seem to be untested and you also mostlikely did not run unit tests locally as prooven by #40154 (comment) So maybe you can now work on one PR after another and get them in bit-by-bit instead of all being worked on in parallel always killing all the CI for everyone else for the rest of the day. Best is to also run some CI locally before pushing:

  • composer run cs:check - To check our coding style - Would catch something like Refactor OC\Server::getAppManager #40113 (comment)
  • composer run cs:fix - To apply automated fixes from above
  • NOCOVERAGE=true TEST_SELECTION=NODB ./autotest.sh sqlite - Run basic unit tests - Would catch something like Refactor OC\Server::getUserManager #40154 (comment)
  • NOCOVERAGE=true ./autotest.sh sqlite - Run unit tests that also interact with the DB (might take a bit longer, so recommending the version without DB for a first test)

This way we can save a lot of CI time with things you can easily check manually locally.

Nothing in 3rdparty or apps has been changed.

  • 3rdparty MUST NOT be changed, it's library code from upstream/3rdparty, not from Nextcloud

  • apps/ would be okay to touch. But I would suggest to wait with that as per above and I would also recommend to got app-by-app there instead of get(*::class)-by-get(*::class). That would:

    1. Reduce chances of conflicts between PRs
    2. Reduce the amount of reviews that are requested per PR
    3. Reduce the things that need testing/looking at for each of your PRs

@summersab
Copy link
Contributor Author

summersab commented Aug 30, 2023

@nickvergessen - oops. I definitely apologize. That wasn't my intention. I had some spare time and thought it would be helpful to do a little housekeeping and clean up some lint. I got into a rhythm and just kept going. I didn't realize that it would block your team from running GitHub Actions.

I will definitely take note of your comments for CI and modifying apps. Also, I promise not to submit any more PRs for the rest of the week - heh . . .

Would it help if I put the unreviewed PRs in draft?

@skjnldsv skjnldsv removed this from the Nextcloud 29 milestone Nov 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing Work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants