search

backdrop / backdrop-issues

Issue tracker for Backdrop core.
144 stars 39 forks source link

The great coding standards restoration #6004

Open ghost opened 1 year ago

ghost commented 1 year ago

Now that we run phpcs against all core PRs, we should fix all existing coding issues in core to avoid getting so many warnings/errors. This issue (inspired by https://github.com/backdrop/backdrop-issues/issues/3213) will list all files/modules/etc. in core and will have PRs against groups of them to make reviewing easier.

Anyone wanting to make a PR should:

ghost commented 1 year ago

One down, a zillion to go!

izmeez commented 10 months ago

Just started to look at core/includes/c and in the first file cache-install.inc encountered 9 problems all related to Visibility must be declared on method. I did a quick search and cannot find information to help figure out what is expected here to fix it. Any help is appreciated.

izmeez commented 10 months ago

I found this page but I am still not able to decipher what is needed, https://www.php-fig.org/psr/psr-12/#44-methods-and-functions

argiepiano commented 10 months ago

This is solved by adding a visibility qualifier in the method declaration such as public, protected or private.

izmeez commented 10 months ago

So since all these 9 problems are contained within class BackdropFakeCache extends BackdropDatabaseCache implements BackdropCacheInterface { within which it is not clear where the method declaration is, where would a fix be added?

izmeez commented 10 months ago

I have found some more specific details here, https://www.php.net/manual/en/language.oop5.visibility.php

izmeez commented 10 months ago

This link provides more details, https://stackoverflow.com/a/4361582

You use:

public scope to make that property/method available from anywhere, other classes and instances of the object. private scope when you want your property/method to be visible in its own class only. protected scope when you want to make your property/method visible in all classes that extend current class including the parent class.

If you don't use any visibility modifier, the property / method will be public.

Thus, since nothing was used before it is probably safe to add public to each of the functions like public function get($cid) {

izmeez commented 10 months ago

I guess this is a time that now a visibility qualifier is being added the code can be "improved" by selecting some thing other than "public" where it might be appropriate. I don't have the depth of knowledge to make that determination.

bugfolder commented 10 months ago

If the function was previously implicitly public, then it would seem that when visibility is made explicit, it should also be public in the interest of backward compatibility.

Some functions might warrant a change in visibility to private or protected, but since that would have B/C implications, it should be raised in a separate issue to discuss the implications, and shouldn't be a blocker for this issue.

izmeez commented 10 months ago

Maybe, the place to raise that would be once there is a pull request. Although there may be a number of instances.

izmeez commented 10 months ago

I also noticed there are other qualifiers like static that may accompany these, such as public static.

izmeez commented 10 months ago

Should I start a PR with what I have done so far even though it is incomplete? So others don't jump into doing the same one?