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

Refactors controllers by using PHP8's constructor property promotion. #4718

Merged
merged 1 commit into from
Jul 14, 2023
Merged

Refactors controllers by using PHP8's constructor property promotion. #4718

merged 1 commit into from
Jul 14, 2023

Conversation

fsamapoor
Copy link
Member

Summary

There are lots of refactoring opportunities and this PR aims to see if it's welcome to make such improvements to the code base.

  • Code is properly formatted
  • Sign-off message is added to all commits

Copy link
Member

@juliushaertl juliushaertl left a comment

Choose a reason for hiding this comment

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

Thanks a lot. I've left a few more comments that would make sense from my perspective if we touch the lines anyways :) Otherwise this would be fine to get in.

lib/Controller/PageController.php Show resolved Hide resolved
lib/Controller/StackApiController.php Outdated Show resolved Hide resolved
lib/Controller/StackApiController.php Outdated Show resolved Hide resolved
lib/Controller/StackController.php Outdated Show resolved Hide resolved
lib/Controller/CardApiController.php Outdated Show resolved Hide resolved
@fsamapoor
Copy link
Member Author

Hello @juliushaertl.

Although not related to changes being made in this PR, I have fixed an error regarding the failed static analysis workflow. After that, I ran the psalm script in my local environment and it finds loads of errors in the project.

In the Controller namespace alone, there are 149 errors found containing only "UndefinedClass" and "UndefinedDocblockClass" errors. I'm new to using Psalm, but to my understanding, those are autoloading-related issues that can be fixed by changing the Psalm configuration file, which is well beyond the scope of this PR.

So, are there any specific steps I need to take in this PR regarding the failed static analysis workflow?

@juliushaertl
Copy link
Member

In the Controller namespace alone, there are 149 errors found containing only "UndefinedClass" and "UndefinedDocblockClass" errors. I'm new to using Psalm, but to my understanding, those are autoloading-related issues that can be fixed by changing the Psalm configuration file, which is well beyond the scope of this PR.

I'm not sure about those, I don't see them locally. For static analysis we pull in the nextcloud/ocp composer package which is a copy of the public API provided by the Nextcloud server. It might be that this is either missing on your setup, depending how you run psalm. Other than that I also had troubles in the past where disabling the psalm caching worked: composer run psalm -- --no-cache

@juliushaertl
Copy link
Member

Would be good to merge from my perspective, can you just do a rebase to latest master to get rid of the merge commit?

@fsamapoor
Copy link
Member Author

Would be good to merge from my perspective, can you just do a rebase to latest master to get rid of the merge commit?

Sorry about that. All good now.

@juliushaertl
Copy link
Member

Failures are likely unrelated as we are just having a test run for more CI workers in the GitHub organization, will trigger them again later

@juliushaertl
Copy link
Member

Would you mind to rebase and squash the commits into one for a clean history?

Co-authored-by: Julius Härtl <[email protected]>
Signed-off-by: Faraz Samapoor <[email protected]>
Signed-off-by: Faraz Samapoor <[email protected]>
@fsamapoor
Copy link
Member Author

@juliushaertl Done.

@juliushaertl juliushaertl merged commit c4a826b into nextcloud:main Jul 14, 2023
23 checks passed
@juliushaertl
Copy link
Member

Awesome. Thanks a lot and sorry for taking so long :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants