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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor get_sites() extension to improve readability #158

Merged
merged 3 commits into from
Feb 26, 2023

Conversation

IanDelMar
Copy link
Contributor

Refactors the dynamic function return type extension for get_sites() to improve readability.

Last PR addressing this extension 馃槆

@szepeviktor
Copy link
Owner

Hello good-old pass-by-ref :(
This is sooo PHP4!

Please use properties for passing data around.

Co-authored-by: Viktor Sz茅pe <viktor@szepe.net>
@IanDelMar
Copy link
Contributor Author

@szepeviktor I switched to properties and now 15 assertions fail. 馃槥 I'll need some time to figure out what's going on.

@herndlm
Copy link
Contributor

herndlm commented Feb 26, 2023

Properties add state to those classes making them mutable. Could be that one line of analysed code sets a property that is then incorrectly used for another line of analysed code?

@IanDelMar
Copy link
Contributor Author

I replaced $count with $this->fields... 馃槅

@szepeviktor szepeviktor merged commit 50153a1 into szepeviktor:master Feb 26, 2023
@IanDelMar IanDelMar deleted the get-sites-refactor branch February 26, 2023 14:03
@herndlm
Copy link
Contributor

herndlm commented Feb 26, 2023

Just one more thing though. A get* method that doesn't return anything but changes some state is one of the things that keeps me from sleeping at night xD

Most likely the simplest and safest thing is just to return the relevant values as an array and type it for phpstan with an array shape. You can use it in a sane way via e.g. [$foo, $bar] = $this->getSomething(). Assuming that Viktor isn't still supporting php and WordPress from 10 years ago here xD

@herndlm
Copy link
Contributor

herndlm commented Feb 26, 2023

OK too slow 馃槄 as long as it works

@szepeviktor
Copy link
Owner

szepeviktor commented Feb 26, 2023

that keeps me from sleeping at night

Mine is get_template_part that is really print_template_part.

@herndlm
Copy link
Contributor

herndlm commented Feb 26, 2023

Oh no, stop it...

@szepeviktor
Copy link
Owner

@herndlm Please star my next gen WordPress: https://github.com/szepeviktor/WordPress-the-good-parts

@IanDelMar
Copy link
Contributor Author

@herndlm you're right. I'll rename the method when I remove the query string argument support (#159).

@herndlm
Copy link
Contributor

herndlm commented Feb 26, 2023

Viktor stop trying to sell me your passive aggressive empty repo 馃槄

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

Successfully merging this pull request may close these issues.

None yet

3 participants