[policy] Define coding standards for anonymous functions (closures)

for what it's worth, in JS we use the spaced version. function (whatever) {}.

pfrenssen: Can you provide examples of both in core, and are they both in use in *our* core code, vs. vendor code? Any idea about which is more prevalent?

Thanks for the examples!

I agree with several commenters above that the space is more in line with our other coding standards. In the second example, it's especially glaring to me that it has function(something) use (something) -- spaces around use but not around function... kind of weird!

On the other hand, when we declare a function like
function foo($bar)
we don't leave a space between the function name and the arguments, so an anonymous function($bar) is kind of like that example too... Could go either way!

Coding standards decisions are now supposed to be made by the TWG

I think it's also not clear (or not yet specified in the coding standards) the style we want to use for a lamda used inline for the callable for a function like array_filter()

This seems to be the style in core - "function" keyword and all the rest up to the opening { on the line with array_filter(), and then all the closing stuff on the final line:

    $tm = array_filter(get_class_methods($class), function ($method) {
      return strpos($method, 'test') === 0;
    });

This seems to be what PSR-2 expects, so I don't really think there is a debate.

I only mention this as needing clarity since I was reviewing a patch that had it formatted quite differently, and I had to guess at a standard based on current usage.

it was more like:

    $tm = array_filter(get_class_methods($class), 
      function ($method) {
        return strpos($method, 'test') === 0;
      }
    );

At the moment (barely any php files may have missed) there are 154 anonymous functions where there space after the function keyword "function (" and 102 anonymous functions where no space "function(".

Always a space after the use keyword (100 of the anonymous functions have lexical variables).

https://github.com/php-fig/fig-standards/blob/master/accepted/PSR-2-codi...

their example is:

$app->get('/hello/{name}', function ($name) use ($app) { 
    return 'Hello '.$app->escape($name); 
});

Which has spaces both paces.

Moving this issue to the Coding Standards queue per the new workflow defined in #2428153: Create and document a process for updating coding standards.

I would vote for this to follow the JavaScript coding standards as outlined by nod_ in #1.

Sounds good to me.

Edit: deleted.

Spaced, +1

PSR-2 specifies a space after the function keyword: https://github.com/php-fig/fig-standards/blob/master/accepted/PSR-2-codi...

Since it looks like we're going that direction anyway on closures, I'd recommend either copying the text from PSR-2 or simply linking to that section of PSR-2. No sense being different when there's no reason to.

The coding standards committee feels this has enough support - sending to core for approval.

Is there an available phpcs rule for this standard?

@xjm, I just added

  $param = '';

  $callback = function($nothing) use ($param) {
    echo $nothing;
  };

to an existing module and got:

536 | ERROR | [x] Expected 1 space after FUNCTION keyword; 0 found

As the response.

Changing this to:

  $param = '';

  $callback = function ($nothing) use ($param) {
    echo $nothing;
  };

Passed without error.

Excellent, this standard is approved then. Thanks @frob.

I filed #2794567: Make core comply with new standard spacing for anonymous functions and enable the rule in phpcs.xml.dist to make core compliant.

Here's an example from ConfigManager: http://cgit.drupalcode.org/drupal/tree/core/lib/Drupal/Core/Config/Confi...

    foreach (['config', 'content'] as $dependency_type) {
      $affected_dependencies[$dependency_type] = array_combine(
        array_map(function ($entity) { return $entity->getConfigDependencyName(); }, $affected_dependencies[$dependency_type]),
        $affected_dependencies[$dependency_type]
      );
    }

Drupal.WhiteSpace.ScopeClosingBrace thinks it should look like this, when phpcbf auto-fixes it:

    foreach (['config', 'content'] as $dependency_type) {
      $affected_dependencies[$dependency_type] = array_combine(
        array_map(function ($entity) { return $entity->getConfigDependencyName();
        }, $affected_dependencies[$dependency_type]),
        $affected_dependencies[$dependency_type]
      );
    }

To me, it should look like this:

    foreach (['config', 'content'] as $dependency_type) {
      $affected_dependencies[$dependency_type] = array_combine(
        array_map(
          function ($entity) {
            return $entity->getConfigDependencyName();
          },
          $affected_dependencies[$dependency_type]
        ),
        $affected_dependencies[$dependency_type]
      );
    }

I bring this up because we have an issue postponed on this very question, and the example is from the patch: #2572795: Fix coding standard for closures - Drupal.WhiteSpace.ScopeClosingBrace and Generic.Functions.OpeningFunctionBraceKernighanRitchie

When this question is answered, we'll need to be sure and file an issue against Coder to support it.

We can't unpostpone #2572795: Fix coding standard for closures - Drupal.WhiteSpace.ScopeClosingBrace and Generic.Functions.OpeningFunctionBraceKernighanRitchie until Coder is fixed. The desire is for the autofix to get it right, because that issue doesn't only fix closures.

@pfrenssen, postponing this doesn't make sense to me. While it is a good goal, to only have coding standards that core has fully conformed to, by not modifying the coding standard doc until after that is done we run the risk of modules/contributors using the wrong code style in the meantime.

AFAIK there aren't that many violations in core, so I'm not too worried one way or the other. It's less confusing for developers than something where core actually does not follow a standard at all, or only half follows it, etc.

That said though, I also don't think modules using the wrong style in the meantime is a risk really. We're not ready to adopt the standard yet. That's okay. We can adopt it with 8.3.0 in six months. We could put a note in the docs that core is not yet compliant, I guess, but it doesn't seem that urgent to me one way or another.

The coder issues don't need to be postponed, for sure.

I only bring it up because I happen to find this issue while looking for the drupal coding standards for anonymous functions.

I would expect that I am not an edge case, as in I would expect that there are other people who have looked for coding standards for anonymous functions and might get confused by our lack of standards on them.

I don't think this is actually fixed until coder and core are done and we officially adopt the standard, as I said in #39. It would be useful to track the issue on the TWG until that's done. Thoughts?

It looks like we don't have an issue to track the docs change other than this one. It would make more sense if we did as this is just labeled as a policy issue. Not sure if there is an established workflow for that.

@xjm filed the implementation issue: #2794567: Make core comply with new standard spacing for anonymous functions and enable the rule in phpcs.xml.dist

Thanks @Mile23! Let's keep this at not-fixed though until that lands or is scheduled.

(Maybe we could have different components for issues that are ratified but not yet adopted or something.)

The implementation for this was done in #2572795: Fix coding standard for closures - Drupal.WhiteSpace.ScopeClosingBrace and Generic.Functions.OpeningFunctionBraceKernighanRitchie and this change is in the release notes for 8.3.0-alpha1.

There is nothing more to do here.

Thanks!

this time really mark it fixed.