Fix Drupal.Commenting.FunctionComment.InvalidNoReturn

Rerolled and also made a few changes so the commit code checks pass locally.

Make changes for #33.

Remove lines from phpstan-baseline.neon

Restore drupalci

Have not reviewed

Seems to have a failure in #43

Seems that I did not restore drupalci.yml as I intended.

At first was confused why it was removing return statement and realized they weren't used haha.

Think this is an excellent cleanup idea and will keep the comments very clean.

The last submitted patch, 45: 2722647-45.patch, failed testing. View results →

Unrelated failure. Retesting

1) Drupal\Tests\menu_ui\Functional\MenuUiTest::testMenuAdministration
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'/subdirectory/admin/structure/menu/manage/x7kqjmks'
+'/subdirectory/admin/structure/menu/manage/x38ro07w'

Tests are passing, restoring RTBC

I'm triaging RTBC issues.

I looked at this late last night and after reading the IS and comments I don't see anything left to do.

Today, I skimmed the code and found this.

+++ b/core/modules/search/src/Form/SearchPageAddForm.php
@@ -42,6 +42,7 @@ public function save(array $form, FormStateInterface $form_state) {
+    // foo.

This looks wrong.

I removed that line and made a new patch.

Removing that unusual comment hasn't cause any test failures. I think it is OK to restore the RTBC here.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

A simple reroll due to changes in the phpstan baseline.

Reroll looks gook. All green.

#54 failed standards checks:

FILE: /var/www/html/core/lib/Drupal/Core/Database/Connection.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 1395 | ERROR | @return doc comment specified, but function has no
      |       | return statement
      |       | (Drupal.Commenting.FunctionComment.InvalidNoReturn)
----------------------------------------------------------------------

Update for changes made in 📌 Refactor transactions Fixed .

It is a simple reroll, restoring RTBC

Wow, got a lot more than I expected here. Thanks everyone for your work on this suprisingly challenging issue.

As I started reviewing this I quickly concluded that the change set is way too broad. We are mixing three things:

1. Simple cleanups where we're declaring null return types which is against the standard.
2. Possible underlying bugs where we're declaring the wrong return types -- each of these should be researched with git blame etc. to validate why the difference between documentation and variables exists, to ensure there's not a bug or an inconsistency stretching into other parts of the codebase. Like unused variable removals, they should get their own issues rather than being part of a bulk cleanup.
3. Actual changes to functionality to return empty string or etc. These changes should come with test coverage!

So, I think we need to split this up. I think there should be:

• One child issue for the the trivial cases in #1
• Another for the DB APIs (since they're all intertangled)
• Various other child issues as we groups that are possibly a single problem in multiple places.
• And so on from there.

Since the only real code reviews so far are mostly related to the issues in the DB API docs, I think maybe we should create a new meta, and convert this to be the child about DB issues, moving the existing code to the meta (I'd created a combined MR on the meta) and then child issues as appropriate.

Also, API docs issues are much easier to review in GitLab than in patches, because the reviewer needs to be able to read the code that the docs are about. With a diff of a docblock in a patch, I have to open the class locally or on api.d.o, find the relevant lines, and compare. On GitLab I can just expand the shown lines in the same interface I'm doing the review. So, let's go with MRs for this and all the other issues we'll split off.

My notes from the code review I started are below:

1. +++ b/core/lib/Drupal/Component/Plugin/PluginManagerBase.php
   @@ -118,9 +118,6 @@ protected function handlePluginNotFound($plugin_id, array $configuration) {
   -   * @return string
   -   *   The id of an existing plugin to use when the plugin does not exist.
   -   *
   
   +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -1392,9 +1392,6 @@ public function transactionManager(): TransactionManagerInterface|FALSE {
   -   * @return \Drupal\Core\Database\Transaction\TransactionManagerInterface
   -   *   The transaction manager.
   

   Unlike most of the changes, this is a place where the documented behavior is different from what actually happens, which could indicate an underlying bug. Have we researched this? (Will check after I post my diff review.) I think this should also be a separate scope, possibly with tests.

   Edit: Same with the DB connection method.

2. +++ b/core/lib/Drupal/Core/Config/ConfigBase.php
   @@ -197,11 +197,12 @@ public function set($key, $value) {
   +   * The function does not return anything but will raise an exception
   +   * if there are issues with keys passed.
   +   *
   
   +++ b/core/lib/Drupal/Core/Config/StorableConfigBase.php
   @@ -140,13 +140,14 @@ protected function getSchemaWrapper() {
   +   * The function does not return anything but will raise an exception if
   +   * the value is unsupported in the configuration.
   

   Isn't this documentation redundant with the @throws? Maybe it's worth explaining because it's odd for the method not to return a status, but OTOH ConfigBase::validateName() has the same behavior already with no docs change here. So, if anything, I think this is out of scope and should be in a followup if we want to improve the docs like so.

3. +++ b/core/lib/Drupal/Core/Database/Query/Merge.php
   @@ -351,6 +351,7 @@ public function key($field, $value = NULL) {
      public function __toString() {
   +    return '';
      }
   

   This is an actual behavior change and seems like it needs its own issue, with tests.

Sorry to be a downer but there is a lot more needed here, and we can get at least parts of it in faster if we split it up. Thanks everyone!

Downer indeed. :-(

Converting to a Meta