Skip to content

Refactor lib/private - #39249

Closed
shdehnavi wants to merge 3 commits into
nextcloud:masterfrom
shdehnavi:refactor_lib_private_root_classes_part_3
Closed

Refactor lib/private#39249
shdehnavi wants to merge 3 commits into
nextcloud:masterfrom
shdehnavi:refactor_lib_private_root_classes_part_3

Conversation

@shdehnavi

@shdehnavishdehnavi commented Jul 9, 2023

Copy link
Copy Markdown
Contributor

Summary

The required adjustments have been made to the following classes under /lib/private namespace:

  • PreviewManager.php
  • Repair.php
  • RedisFactory.php
  • Search.php
  • Server.php
  • ServerContainer.php

The improvements:

  • Using PHP8's constructor property promotion
  • Adding return types
  • Adding types to properties
  • Updating doc blocks
  • Converting if (isset(...)) to null coalescing operator

Checklist

Comment threadlib/private/Server.php Fixed
@solracsfsolracsf added 3. to review Waiting for reviews technical debt 🧱 🤔🚀 labels Jul 9, 2023
@solracsfsolracsf added this to the Nextcloud 28 milestone Jul 9, 2023
@shdehnavishdehnavi mentioned this pull request Jul 13, 2023
@fsamapoor

Copy link
Copy Markdown
Member

Could you please resolve the conflicts?

Signed-off-by: Hamid Dehnavi <hamid.dev.pro@gmail.com>
Signed-off-by: Hamid Dehnavi <hamid.dev.pro@gmail.com>
@shdehnavi
shdehnaviforce-pushed the refactor_lib_private_root_classes_part_3 branch from 083a390 to f837b57CompareAugust 16, 2023 06:30
@skjnldsvskjnldsv mentioned this pull request Nov 1, 2023
This was referenced Nov 6, 2023
This was referenced Nov 14, 2023
@blizzzblizzz modified the milestones: Nextcloud 28, Nextcloud 29Nov 23, 2023
This was referenced Mar 12, 2024
This was referenced Mar 20, 2024
@skjnldsvskjnldsv mentioned this pull request Mar 28, 2024
81 tasks
@skjnldsv
skjnldsvforce-pushed the refactor_lib_private_root_classes_part_3 branch from be88bf5 to c3ca53fCompareMay 2, 2024 15:36
@skjnldsv
skjnldsv requested review from a team and removed request for a teamMay 2, 2024 15:39
@skjnldsv
skjnldsvforce-pushed the refactor_lib_private_root_classes_part_3 branch from c3ca53f to 876fbbeCompareMay 2, 2024 15:39
Signed-off-by: John Molakvoæ <skjnldsv@users.noreply.github.com>
@skjnldsv
skjnldsvforce-pushed the refactor_lib_private_root_classes_part_3 branch from 876fbbe to 98ef21fCompareMay 2, 2024 15:41
* @return array|null
*/
protected function getEnabledDefaultProvider() {
protected function getEnabledDefaultProvider(): ?array {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
protectedfunction getEnabledDefaultProvider(): ?array {
protectedfunction getEnabledDefaultProvider(): array {

I do not see where it returns null?
Docblock @return should be reverted to array as well or removed.

private SystemConfig $config;

private IEventLogger $eventLogger;
private \Redis|\RedisCluster $instance;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This cannot work because the property is not instanciated in constructor.
Most likely should be changed to:

Suggested change
private \Redis|\RedisCluster $instance;
private \Redis|\RedisCluster|null$instance;

And then the class should be adapted so that psalm is happy. The create method is used for creating the object.
The instanceof test in getInstance looks wrong as it does not test cluster.

* @deprecated 20.0.0
*/
public function getCalendarManager() {
public function getCalendarManager(): \OCP\Calendar\IManager {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not sure it makes sense to strong type these obsolete methods.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

deprecated since 20 means we can even remove it when not actively used in shipped apps or server :P

* @deprecated 20.0.0
*/
public function getBruteForceThrottler() {
public function getBruteForceThrottler(): Throttler {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
publicfunction getBruteForceThrottler(): Throttler {
publicfunction getBruteForceThrottler(): IThrottler {

@skjnldsvskjnldsv added 2. developing Work in progress stale Ticket or PR with no recent activity and removed 3. to review Waiting for reviews labels Jul 27, 2024
This was referenced Jul 30, 2024
@skjnldsvskjnldsv closed this Aug 3, 2024
@skjnldsvskjnldsv removed this from the Nextcloud 30 milestone Aug 14, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

2. developingWork in progressstaleTicket or PR with no recent activitytechnical debt🧱 🤔🚀

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@shdehnavi@fsamapoor@nickvergessen@skjnldsv@github-advanced-security@come-nc@blizzz@solracsf