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 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.

By directory

• [ ] core/includes/a*: https://github.com/backdrop/backdrop/pull/4369
• [ ] core/includes/b*: https://github.com/backdrop/backdrop/pull/4370
• [ ] core/includes/c*
• [x] core/includes/database/*: https://github.com/backdrop/backdrop/pull/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 https://github.com/backdrop/backdrop/pull/4962
• [x] CSpell https://github.com/backdrop/backdrop-issues/issues/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 https://github.com/backdrop/backdrop/pull/4915

[indigoxela]

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

[indigoxela]

Tentatively marking https://github.com/backdrop/backdrop/pull/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. :+1:

[quicksketch]

Thank you @indigoxela! I think the changes are low-risk. I merged into 1.x and cherry-picked into 1.29.x, so https://github.com/backdrop/backdrop/pull/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 https://github.com/backdrop/backdrop/pull/4962.

EDIT: Second PR that uses protected instead of public for more methods https://github.com/backdrop/backdrop/pull/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 https://github.com/backdrop/backdrop/pull/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 https://github.com/backdrop/backdrop/pull/4962, since an unspecified method like function foo() is considered the same as public function foo().

[herbdool]

I've got a PR for core/includes/a* to replace the previous one where the original repo was deleted: https://github.com/backdrop/backdrop/pull/5167.

For some reason I cannot update the original issue body text. I have permission to do this for any other issue in core, but not this one.

[herbdool]

PR for core/includes/b*: https://github.com/backdrop/backdrop/pull/5168. Replaces: https://github.com/backdrop/backdrop/pull/4370

[quicksketch]

Merged core/includes/a* https://github.com/backdrop/backdrop/pull/5167 into 1.x and 1.32.x. Thanks @herbdool!

[quicksketch]

For some reason I cannot update the original issue body text. I have permission to do this for any other issue in core, but not this one.

Strange, I cannot either. Maybe this is some kind of safety thing for deleted accounts? @BWPanda opened this issue. I can still check boxes off, I just can't edit the summary.

[indigoxela]

Hey @quicksketch and @herbdool can we please pause here and try to get https://github.com/backdrop/backdrop-issues/issues/7008 in first? 🥺

The latest commit (besides causing a conflict) introduced yet another "deprecated" nagging. I belief, getting full PHP 8.5 support is more urgent than coding standards. And with PHP 8.5 we'd immediately see such problems in functional test runs.

[quicksketch]

Ofcourse @indigoxela! Good call!

[herbdool]

This PR for core/includes/c* can wait, though I've confirmed it doesn't currently touch any of the files currently in the PHP 8.5 support PR: https://github.com/backdrop/backdrop/pull/5170.

[indigoxela]

I've confirmed it doesn't currently touch any of the files currently in the PHP 8.5 support PR...

What triggered my comment here was, that the previous merge introduced another "deprecated implicit null" problem that wasn't there before. Not so much the merge conflict, which was trivial. However, the merge conflict was about a doc block I set correctly in my PR, but was misleading in the merged PR here. 😉

Please don't get me wrong, I'm really excited about the activity here. But let's try to not cause additional work elsewhere. 😆