Problem/Motivation

It is sometimes necessary to have empty methods and functions. For example in the Containter class:

  /**
   * Ensure that cloning doesn't work.
   */
  private function __clone()
  {
  }

Or for a procedural function (this only should happen in unit tests):

if (!function_exists('drupal_set_message')) {
  function drupal_set_message() {}
}

At the moment Coder allows say that an empty method in class should be formatted like:

Class

  private function __clone() {}

or

  private function __clone() {
  }

Trait

  private function __clone() {
  }

Procedural function

if (!function_exists('drupal_set_message')) {
  function drupal_set_message() {
  }
}

Proposed resolution

Determine a consistent coding standard for empty functions.

Current proposal

Regardless of whether we are dealing with a class, trait or function if empty the declaration should appear like:

  private function __clone() {
  }

This is because if any functionality is added later to method or function in the diff you will see only additions which is exactly what has occurred. If we allow private function __clone() {} the diff will contain unnecessary change.

Remaining tasks

  • Discuss proposal
  • Determine if and how coding standards should be updated
  • Fix coder to comply
  • Apply to core

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Issue summary: View changes
andypost’s picture

andypost
he/him
Primary language Russian
commented

IMO better to have inline comment inside empty body explaining why this function is no-op

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@andypost sure a comment is good but I really don't think that we should mandate that in a coding standard. As the clone example in the issue summary shows the function or method doc block can take care of that. And a comment in the test code is OTT too.

mfernea’s picture

mfernea commented

I just want to mention that currently Coder allows both styles for classes:

private function __clone() {}

and

private function __clone() {
}

but only the second one for traits or procedural code.

I would go for the second one in all cases.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Issue summary: View changes

Updated IS to reflect @mfernea's comment in #5

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented

+1, let's have a consistent standard for this.
I don't really care which of the two it is, though. I think it looks cleaner without a newline, and it's more obvious that the body is empty; but the diff reasoning also makes sense, and has the advantage of not being purely a matter of personal taste. So, I guess, let's go with that.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented

I agree with everything in #7, slight preference for the version without the newline.

zaporylie’s picture

zaporylie
he/him
Primary language Polish
commented

I'm leaning towards version without newline.

effulgentsia’s picture

effulgentsia commented

We quickly looked at this during our Coding Standards call today, but given that people commenting so far seem split on whether they like the newline or not, we think this needs more people advocating for it before we try to move it forward in the process.

megachriz’s picture

megachriz
Primary language Dutch
commented

+1
I too prefer the version without the newline. But I'm not strongly against the version with the newline.

gapple’s picture

gapple
he/they
Primary language English
commented

Related but slightly different - empty catch blocks to ignore an exception

try {
  // Something that might throw an exception that's safe to ignore.
}
catch (\Exception $e) {}

It looks like there are currently instances in core where a comment is placed above the block, with only a newline inside

try {
  // Something that might throw an exception that's safe to ignore.
}
// Why this exception is being ignored.
catch (\Exception $e) {
}

and instances where the comment is inside the block

try {
  // Something that might throw an exception that's safe to ignore.
}
catch (\Exception $e) {
  // Why this exception is being ignored.
}

(Most comments look like they provide at least some context, but at least one is just "Nothing to do.")

pfrenssen’s picture

pfrenssen
Location Sofia
commented

This is more relevant nowadays with PHP 8 constructor property promotion that will result in many empty constructors, for example:

  /**
   * Constructs a new Foo plugin.
   *
   * @param \Drupal\Core\Render\RendererInterface $renderer
   *   The renderer.
   */
  public function __construct(
    protected RendererInterface $renderer
  ) {}

My personal preference is also to the version without newline.