The great coding standards restoration

[ghost]

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 #3213) will list all files/modules/etc. in core and will have PRs against groups of them to make reviewing easier.

By directory

• core/includes/a*: Issue #6004: Fix coding standards. backdrop#4369
• core/includes/b*: Issue #6004: Fix coding standards. backdrop#4370
• core/includes/c*
• core/includes/database/*: Issue #6004: Coding standards cleanup for database system. backdrop#4915
• core/includes/d*
• core/includes/e*
• core/includes/f*
• core/includes/[g-i]*
• core/includes/[l-m]*
• core/includes/p*
• core/includes/[s-t]*
• core/includes/transliteration*
• core/includes/u*
• core/modules/admin_bar
• core/modules/block
• core/modules/book
• core/modules/ckeditor
• core/modules/color
• core/modules/comment
• core/modules/comment/views
• core/modules/config
• core/modules/contact
• core/modules/contextual
• core/modules/dashboard
• core/modules/date
• core/modules/date/views
• core/modules/[dblog,email]
• core/modules/entity
• core/modules/entityreference
• core/modules/field_ui
• core/modules/field
• core/modules/field/modules
• core/modules/field/[tests,views]
• core/modules/file
• core/modules/file/views
• core/modules/filter
• core/modules/image
• core/modules/installer
• core/modules/language
• core/modules/layout
• core/modules/layout/includes
• core/modules/layout/plugins
• core/modules/layout/[templates,tests]
• core/modules/link
• core/modules/locale
• core/modules/local/views
• core/modules/menu
• core/modules/node
• core/modules/node/views
• core/modules/path
• core/modules/redirect
• core/modules/search
• core/modules/search/[templates,tests,views]
• core/modules/simpletest
• core/modules/simpletest/tests/[a-b]*
• core/modules/simpletest/tests/c*
• core/modules/simpletest/tests/d*
• core/modules/simpletest/tests/[e-f]*
• core/modules/simpletest/tests/[g-m]*
• core/modules/simpletest/tests/[n-s]*
• core/modules/simpletest/tests/t*
• core/modules/simpletest/tests/u*
• core/modules/syslog
• core/modules/system
• core/modules/system.system.[a-m]*
• core/modules/system.system.[n-z]*
• core/modules/taxonomy
• core/modules/taxonomy/views
• core/modules/telemetry
• core/modules/translation
• core/modules/update
• core/modules/user
• core/modules/user/tests
• core/modules/user/views
• core/modules/views_ui
• core/modules/views
• core/modules/views/handlers/views_handler_a*
• core/modules/views/handlers/views_handler_field*
• core/modules/views/handlers/views_handler_filter*
• core/modules/views/handlers/views_handler_[r-s]*
• core/modules/views/includes
• core/modules/views/plugins/views_plugin_a*
• core/modules/views/plugins/views_plugin_[c-d]*
• core/modules/views/plugins/views_plugin_[e-q]*
• core/modules/views/plugins/views_plugin_[r-s]*
• core/modules/views/templates
• core/modules/views/tests
• core/modules/views/tests/[handlers,plugins,styles]
• core/profiles
• core/themes
• [misc.]

By topic

• SimpleTest method scope Issue #6004: Add scoping to all test methods to bring inline with coding standards. backdrop#4962
• CSpell [ONGOING] CSpell: Fix nags and clean dictionary up #6302

Anyone wanting to make a PR should:

• Setup their local with phpcs installed and Backdrop's Coding Standards repo one directory above Backdrop core (I personally use Lando)
• Run phpcs from Backdrop's root directory like so: phpcs --basepath=. --standard=../phpcs/Backdrop core/includes/a* (change the last bit to match whatever group of files you're targeting)
• Create their PR with the following description: For https://github.com/backdrop/backdrop-issues/issues/6004 (avoiding any keywords that automatically link the PR to this issue and therefore result in closing this issue when the PR is merged)
• Edit this issue to add a link to your PR next to the appropriate item above

[ghost]

One down, a zillion to go!

[izmeez]

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]

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]

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

[izmeez]

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]

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

[izmeez]

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]

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]

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]

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

[izmeez]

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

[izmeez]

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?

[quicksketch]

I took a stab at cleaning up the core/includes/database directory, since the database system is both one of the most complicated and difficult systems in Backdrop, and also the more poorly documented. Big swaths of functions don't include docblocks. I did not get phpcs passing 100%, since adding array type hints to parent classes and interfaces can break child classes. I also avoided a few places where conforming to code style might result in different behavior. Still, it's hundreds of code style corrections.

See PR at backdrop/backdrop#4915

[indigoxela]

Although it looked like a huge task, backdrop/backdrop#4915 seems to move fast. Some more "but this can also be null" comments 😉, but I'm done now with review. (And phpcs found some indentation issues.)

[indigoxela]

Tentatively marking backdrop/backdrop#4915 as RTBC - phpcs checks pass. There are still some open comments re method removal/addition, but consensus seems to be, that these changes are harmless.

@quicksketch many thanks for sacrificing your weekend to improve the database abstraction layer documentation. 👍

[quicksketch]

Thank you @indigoxela! I think the changes are low-risk. I merged into 1.x and cherry-picked into 1.29.x, so backdrop/backdrop#4915 changes will be in 1.29.3

[quicksketch]

One of the most frequent PHPCS issues we get is scoping on SimpleTest methods, and that frequently confuses contributors who don't know whether tests should be public/protected/private. I tackled specifically that issue with a find/replace, updating 1883 methods (gulp) that did not have scoping on them.

PR ready for review at backdrop/backdrop#4962.

EDIT: Second PR that uses protected instead of public for more methods backdrop/backdrop#4963

[izmeez]

I'm confused by the two PR's. They do not look like they can be combined. Is one preferred over the other?

[quicksketch]

They do not look like they can be combined. Is one preferred over the other?

They're total alternatives to each other, only one could be used. I think backdrop/backdrop#4963 might be the more technically correct solution, but it might not be backwards-compatible. We need to do some testing to see if it would break any other tests (such as those in contrib). If that's the case, then we'd need to go with backdrop/backdrop#4962, since an unspecified method like function foo() is considered the same as public function foo().