[Meta] Fix Drupal.Commenting.FunctionComment.InvalidNoReturn

Ooo a thorny one. :-)

+++ b/core/lib/Drupal/Core/Config/ImmutableConfig.php
@@ -49,6 +49,7 @@ public function save($has_trusted_data = FALSE) {
   public function delete() {
     throw new ImmutableConfigException("Can not delete immutable configuration {$this->getName()}. Use \\Drupal\\Core\\Config\\ConfigFactoryInterface::getEditable() to retrieve a mutable configuration object");
+    return $this;
   }
 

+++ b/core/lib/Drupal/Core/Database/Connection.php
@@ -173,7 +173,9 @@ public function __construct(\PDO $connection, array $connection_options) {
    * @return \PDO
    *   A \PDO object.
    */
-  public static function open(array &$connection_options = array()) { }
+  public static function open(array &$connection_options = array()) {
+    return $this->connection;
+  }

+++ b/core/lib/Drupal/Core/FileTransfer/FileTransfer.php
@@ -75,6 +75,7 @@ function __construct($jail) {
    */
   static function factory($jail, $settings) {
     throw new FileTransferException('FileTransfer::factory() static method not overridden by FileTransfer subclass.');
+    return new stdClass();
   }

+++ b/core/modules/menu_link_content/src/Plugin/Menu/MenuLinkContent.php
@@ -198,7 +198,7 @@ public function getTranslateRoute() {
    *   The menu link ID.
    */
   protected function getUuid() {
-    $this->getDerivativeId();
+    return $this->getDerivativeId();
   }

+++ b/core/modules/node/node.module
@@ -470,7 +470,7 @@ function node_revision_load($vid = NULL) {
  *   TRUE if the revision deletion was successful; otherwise, FALSE.
  */
 function node_revision_delete($revision_id) {
-  entity_revision_delete('node', $revision_id);
+  return entity_revision_delete('node', $revision_id);
 }

If these code changes are made to match the documentation, then they're bugs and should be a separate issue against 8.1.x.

I'm really kind of amazed that tests passed with these changes, for at least two reasons. :-)

Beyond that, the doc changes will require careful examination of the methods to review.

Let's just postpone this until the other issues are fixed, because we shouldn't patch phpcs.xml.dist before they are.

This is also blocked by #2906931: False positives for Drupal.Commenting.FunctionComment.InvalidNoReturn - Yield statement.

The blocking issues are committed, so this issue is now unblocked.

Tried to re-roll the patch from #2.

Thanks for the reroll.

I ran phpcs across the whole codebase and found a few more issues and tried to fix them. Two issues remain:

For the case of \Drupal\Core\FileTransfer\FileTransfer:

  public static function factory($jail, $settings) {
    throw new FileTransferException('FileTransfer::factory() static method not overridden by FileTransfer subclass.');
  }

Can't we just declare this method abstract?

In \Drupal\Core\Database\Schema:

  /**
   * Finds the columns for the primary key, unique keys and indexes of a table.
   *
   * @param string $table
   *   The name of the table.
   *
   * @return array
   *   A schema array with the following keys: 'primary key', 'unique keys' and
   *   'indexes', and values as arrays of database columns.
   *
   * @throws \Drupal\Core\Database\SchemaObjectDoesNotExistException
   *   If the specified table doesn't exist.
   * @throws \RuntimeException
   *   If the driver does not implement this method.
   */
  protected function introspectIndexSchema($table) {
    if (!$this->tableExists($table)) {
      throw new SchemaObjectDoesNotExistException("The table $table doesn't exist.");
    }
    throw new \RuntimeException("The '{$this->connection->driver()}' database driver does not implement " . __METHOD__);
  }

I'm not sure what to do here, as adding a dead-code return seems wrong as well.

1. +++ b/core/lib/Drupal/Component/PhpStorage/FileStorage.php
   @@ -88,9 +88,6 @@ protected function ensureDirectory($directory, $mode = 0777) {
   -   *
   -   * @return bool
   -   *   TRUE if the directory exists or has been created, FALSE otherwise.
   

   I do not see why this change is needed. The method returns a boolean value.

2. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -271,7 +271,9 @@ public function __construct(\PDO $connection, array $connection_options) {
   +  public static function open(array &$connection_options = []) {
   +    return $this->connection;
   +  }
   

   This change is wrong. $this->connection is not a PDO object. Lets make the method an abstract method as every by core supported driver implements the method as does the ones for SQL Server and OracleDB. Also, the method now does not work for any driver. Lets change the code to: abstract public static function open(array &$connection_options = []);.

3. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
   @@ -1452,9 +1452,6 @@ protected function addTableDefaults(&$schema) {
       * @param array $schema
   ...
      protected function processDataTable(ContentEntityTypeInterface $entity_type, array &$schema) {
   
   @@ -1469,9 +1466,6 @@ protected function processDataTable(ContentEntityTypeInterface $entity_type, arr
       * @param array $schema
   ...
      protected function processRevisionDataTable(ContentEntityTypeInterface $entity_type, array &$schema) {
   

   Can we change these parameter documentation to: @param array &$schema.

4. +++ b/core/modules/options/src/Plugin/Field/FieldType/ListItemBase.php
   @@ -219,7 +219,9 @@ protected static function extractAllowedValues($string, $has_data) {
       * @return string
   ...
   +    return NULL;
   

   As we are returning null here the return value in the docblock must be changed to: @return string|null.

5. For the case of \Drupal\Core\FileTransfer\FileTransfer:

     public static function factory($jail, $settings) {
       throw new FileTransferException('FileTransfer::factory() static method not overridden by FileTransfer subclass.');
     }
   

   Can't we just declare this method abstract?

   +1 for me. In its docblock it is saying: "lasses that extend this class must override the factory() static method. They should return a new instance of the appropriate FileTransfer subclass.".

6. As for the method \Drupal\Core\Database\Schema::introspectIndexSchema(). The method is not usable without every database driver overridding it. Every by core suported driver does so and the same for SQL Server and OracleDB. Lets change the method to an abstract one.

Maybe instead of removal of return null, use void?

Added reroll of patch #21.

@andypost PHPDoc standards say "@return void" can be omitted when methods do not return anything: https://docs.phpdoc.org/3.0/guide/references/phpdoc/tags/return.html#return

Addressed all comments in #22, also cleaned up one more docblock to use inheritdoc instead of "Overrides..." and reworded the ImageTest mock setup methods as they don't "Get" anything really.

We should include a full stop on this comment
The link definitions to be altered. Each link has a key that is the machine
It can be a problem because with 1 more char it'll have more than 80 chars and we need to split in a second line

@RenatoG I don't think a full stop needs adding, the first line is being replaced but the sentence continues on the next line:

 *   The link definitions to be altered. Each link has a key that is the machine
 *   name, which must be unique. By default, use the route name as the

Implemented two abstract methods in test classes.

Yes, it's true @longwave/ It really makes sense

#30 looks good

It could use follow-up looking on winning never for PHP 8.1 https://wiki.php.net/rfc/noreturn_type

+++ b/core/lib/Drupal/Core/Database/Schema.php
@@ -520,15 +520,8 @@ protected function findPrimaryKeyColumns($table) {
-   * @throws \RuntimeException
-   *   If the driver does not implement this method.
    */
-  protected function introspectIndexSchema($table) {
-    if (!$this->tableExists($table)) {
-      throw new SchemaObjectDoesNotExistException("The table $table doesn't exist.");
-    }
-    throw new \RuntimeException("The '{$this->connection->driver()}' database driver does not implement " . __METHOD__);
-  }
+  abstract protected function introspectIndexSchema($table);

+++ b/core/lib/Drupal/Core/FileTransfer/FileTransfer.php
@@ -73,9 +70,7 @@ public function __construct($jail) {
-  public static function factory($jail, $settings) {
-    throw new FileTransferException('FileTransfer::factory() static method not overridden by FileTransfer subclass.');
-  }
+  abstract public static function factory($jail, $settings);

These are API changes that we shouldn't be making in a coding standards change. The FileTransferException seems more okay but it should get its own issue. The introspectIndexSchema is definitely not okay. This method exists only for testing purposes - see SchemaIntrospectionTestTrait - that's the only place this stuff is called. There's no need for a contrib driver to declare this method and making this change could break a contrib driver.

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.

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 #3364706: Refactor transactions.

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 were declaring null return types which is against the standard but still accurate docs.
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

Thank you to everyone who helped to complete this issue. The last child was recently committed. So, I am closing this as fixed.

Amending attribution.