Skip to content

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

Merged
juliusknorr merged 1 commit into
nextcloud:mainfrom
fsamapoor:constructor_property_promotion_in_controllers
Jul 14, 2023
Merged

juliusknorr merged 1 commit into
nextcloud:mainfrom
fsamapoor:constructor_property_promotion_in_controllers

Conversation

@fsamapoor

Copy link
Copy Markdown
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

@juliusknorr juliusknorr left a comment

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.

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.

Comment thread lib/Controller/PageController.php Outdated
Comment thread lib/Controller/StackApiController.php Outdated
Comment thread lib/Controller/StackApiController.php Outdated
Comment thread lib/Controller/StackController.php Outdated
Comment thread lib/Controller/CardApiController.php Outdated
@fsamapoor

Copy link
Copy Markdown
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?

@juliusknorr

Copy link
Copy Markdown
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

@juliusknorr

Copy link
Copy Markdown
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
Copy Markdown
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.

@juliusknorr

Copy link
Copy Markdown
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

@fsamapoor
fsamapoor requested a review from juliusknorr June 27, 2023 16:24
@juliusknorr

Copy link
Copy Markdown
Member

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

Co-authored-by: Julius H盲rtl <jus@bitgrid.net>
Signed-off-by: Faraz Samapoor <f.samapoor@gmail.com>
Signed-off-by: Faraz Samapoor <fsa@adlas.at>
@fsamapoor

Copy link
Copy Markdown
Member Author

@juliushaertl Done.

@juliusknorr
juliusknorr merged commit c4a826b into nextcloud:main Jul 14, 2023
@juliusknorr

Copy link
Copy Markdown
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