Problem/Motivation

Easy fixes for implementing the sniff, Drupal.Commenting.FunctionComment.InvalidNoReturn.

Steps to reproduce

Proposed resolution

Adjust comment for what the method actually does. Some are hooks that do not return anything.
Also changes for tests

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3387988

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

quietone created an issue. See original summary.

quietone’s picture

quietone commented

These are not changes that are not hooks and the issue that added the @return statement. Skimming those issue I did not see anything that suggests these should be returning a value.

\Drupal\Core\Config\ImmutableConfig::delete : added in #2392319: Config objects (but not config entities) should by default be immutable. The return type added in first patch where it was like it is now, only throwing an exception.
\Drupal\rest\Routing\ResourceRoutes::onDynamicRouteEvent : #2158571: Routes added in RouteSubscribers cannot be altered in #134.
\Drupal\locale\PluralFormula::loadFormulae : #2571375: Remove TranslationManager dependency from LanguageManager in #19
TemporaryArrayObjectThrowingExceptions : #3232074: [Symfony 6] Add "array|string|int|float|bool|\ArrayObject|null" to all Normalizer classes that implement the method ::normalize()

quietone opened merge request !4810

quietone’s picture

quietone commented
Title: Fix @return type, documentation only » Fix @return type, simple fixes
quietone’s picture

quietone commented
Issue summary: View changes

Add SqlContentEntityStorageSchema. The return type was added in #2183231-34: Make ContentEntityDatabaseStorage generate static database schemas for content entities and was later committed. The function never had a return statement.

quietone’s picture

quietone commented
Status: Active » Needs review
atul4drupal’s picture

atul4drupal commented
Status: Needs review » Needs work

1) core/modules/rest/tests/src/Functional/ResourceTestBase.php

protected static function recursiveKSort(array &$array) {
    // First, sort the main array.
    ksort($array);

    // Then check for child arrays.
    foreach ($array as $key => &$value) {
      if (is_array($value)) {
        static::recursiveKSort($value);
      }
    }

    return $array;
  }

}

This function still returns an array and I think it will not be appropriate to remove the @return comment for this function until the function is updated to not return anything (As this function is updating a variable passed by reference this may be updated to not return anything in a separate issue and comment may be updated accordingly too).

quietone’s picture

quietone commented
Status: Needs work » Needs review

@atul4drupal, thanks.

I reverted that change and created a new issue for ResourceTestBase, #3388204: Fix return type in \Drupal\Tests\rest\Functional\ResourceTestBase::recursiveKSort()

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Seems there are some test failures.

Have not reviewed

quietone’s picture

quietone commented
Status: Needs work » Reviewed & tested by the community

The failures are unrelated. I rebased the MR via GitLab and tests are passing now. Therefore, I am restoring RTBC.

1) Drupal\FunctionalJavascriptTests\Ajax\ThrobberTest::testProgressThrobberPosition
Behat\Mink\Exception\ElementNotFoundException: Element matching xpath "//div[contains(@class, "dropbutton-wrapper")]/following-sibling::div[contains(@class, "ajax-progress-throbber")]" not found.
1) Drupal\Tests\toolbar\FunctionalJavascript\ToolbarActiveTrailTest::testToolbarActiveTrail with data set #1 ('horizontal')
Failed asserting that false is true.
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Needs work

I think the change to hook_theme_suggestions_alter() needs a bit more work while we are here, but otherwise I checked each case and they all look correct to me.

quietone’s picture

quietone commented
Status: Needs work » Needs review
Issue tags: +Coding standards

Adding tag.

There are no code changes and a followup has been made so I am restoring the RTBC.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Per #12

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Fixed

Thanks for opening the followup.

Committed and pushed to 11.x, 10.2.x and 10.1.x.

  • longwave committed 457fb8bc on 10.1.x 
    Issue #3387988 by quietone, atul4drupal: Fix @return type, simple fixes...

  • longwave committed 3410fb54 on 10.2.x 
    Issue #3387988 by quietone, atul4drupal: Fix @return type, simple fixes...

  • longwave committed 5e8d5e8a on 11.x 
    Issue #3387988 by quietone, atul4drupal: Fix @return type, simple fixes

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

quietone closed merge request !4810