Skip to content

Loading…

PSR-2 : clarification on multi-line function calls #61

Closed
BenMorel opened this Issue · 28 comments

9 participants

@BenMorel

The PSR-2 standard defines clearly how function arguments can be split across multiple lines:

function test(
    $a,
    $b
) {
    // ...
}

test(
    1,
    2
);

However, it's unclear whether the following is acceptable:

test(foo(
    1,
    2
));

test(array(
    1,
    2
));

test(function() {
    // ...
});

It feels acceptable to me, as there is just one argument to the function test(), but the newlines in between make it a bit confusing.

The current implementation of PSR-2 in PHP_CodeSniffer reports the following errors for each of the examples above:

Opening parenthesis of a multi-line function call must be the last content on the line
Closing parenthesis of a multi-line function call must be on a line by itself

As this contradicts with my own interpretation of the standard, I'm turning to you guys, to get a clarification on this, and maybe reflect this clarification in the standard itself!

Thanks in advance for your comments.

@Seldaek

I would tend to agree with you, especially since I believe it's not specced. I don't think PHPCS should check those.

@dragoonis dragoonis closed this
@hinikato

I have created a new issue for the phpcs also: http://pear.php.net/bugs/bug.php?id=19768
I think the asked above question by BenMore must be clarified in the PSR-* because it is unclear what format must be used, the:

new Site(array(
     'name' => $siteName,
     'dirPath' => $this->getAllSitesDirPath() . '/' . $siteName,
)); 
// and one more notation:
new Site([
     'name' => $siteName,
     'dirPath' => $this->getAllSitesDirPath() . '/' . $siteName,
]); 

or the next one:

new Site(
    array(
         'name' => $siteName,
         'dirPath' => $this->getAllSitesDirPath() . '/' . $siteName,
    )
);
// and one more notation:
new Site(
    [
         'name' => $siteName,
         'dirPath' => $this->getAllSitesDirPath() . '/' . $siteName,
    ]
);

P.S. My personal opinion that the first more short notation is more convenient and must be used.

@BenMorel

@dragoonis, could you comment on why you closed the issue?

@dragoonis
PHP FIG member

Must have been an accident, or closed the wrong one. re-opened carry on :+1:

@dragoonis dragoonis reopened this
@BenMorel

Thanks! Waiting for the comments of other participants then.

@BenMorel

I wrote a pretty naive pattern checker to survey a sample of the projects mentioned in the original PSR survey.
Here are the results for these two pattern checks:

Number of occurrences : 8152

foo(bar(
    // ...

Number of occurrences : 2426

foo(
    bar(
        // ..

The test is overly simple, in particular because it doesn't check how many elements come on the following lines.
But still, it would seem to show a strong preference for the first interpretation of the standard above.

The implementation is available here on GitHub, you're welcome to improve it, or to suggest to use a better tool for the job.

Here comes the survey data, with the URL of the archive / repository used:

@hinikato

We need more example in the PSR that should clarify this case because there is a confusion here.

@localheinz

@BenMorel

It should be

// note the trailing comma
test(array(
    1,
    2,
));

// note space
test(function () {
    // ...
});
@localheinz

Apart from that, I agree with @hinikato.

@localheinz

For reference, also see this: zendframework/zf2#4390.

Ping @padraic.

@hinikato hinikato added a commit that referenced this issue
@hinikato hinikato Issue #61: Section 4.6, clarification for multi-line function calls
Fixes for pull request.
e388ec5
@hinikato

Okay here is respective additions (see pull request).

@BenMorel

@localheinz I agree for the spaces before the parentheses, but where did you find mention of the trailing comma? The only relevant example I've found in the PSR-2 spec is:

$foo->bar(
    $longArgument,
    $longerArgument,
    $muchLongerArgument
);

... and there is no trailing comma (which looks weird to me, by the way).

@localheinz

In the example, the method test is fed a single argument - an array with two elements. In my opinion, it's best practice to use a trailing comma when initializing an array and spreading the elements across multiple lines. Think version control and diffs.

It may not be part of PSR-2, but I'm sure it's been part of the discussions towards it.

For me, it's a natural thing.

@phpwalter
@localheinz
@BenMorel

Anyway, this is not the subject of this thread. Please open another issue if you want to discuss this, as your comments just mess up this issue.

@localheinz

@BenMorel, you asked for comments.

@BenMorel

... because you said that it should be something that's not spec'd.

@philsturgeon

This has ben discussed a few times, and as conversation is not progressing I'll summarize things.

  • PSR-2 is vague about this - some say intentionally vague - but as there is no rule about it, its personal preference.
  • PHPCS is over-zealous, and decides that one way or the other is correct. If you want to use PHPCS, you'll have to go with its rules, but PSR-2 itself doesn't care.
  • Changing PSR-2 is not going to happen.
@philsturgeon

There is an ongoing conversation about this and the action will most likely be two part:

  1. Trying to get Errata put into PSRs somehow (maybe in the Meta Doc).
  2. Asking the PHP CS team to be less strict.
@BenMorel

@philsturgeon That would be great!

@philsturgeon

If this poll looks good I'll be making a PR outlining this stuff. I'll post a link here when its done.

@BenMorel

Great, thanks for carrying on the discussion.

@bobthecow bobthecow referenced this issue in bobthecow/psysh
Closed

Adding support for named constants #11

@alcalyn

An other thing not clear to me is the following structure I use often:

return $this->_em->createQuery('
    select count(p.id)
    from CoreBundle:Player p
    where p.pseudo = :pseudo
    and p.invited = 0
')
->setParameters(array(
    'pseudo' => $pseudo,
))
->getSingleScalarResult();

Throws the psr2 error:

 18 | ERROR | Opening parenthesis of a multi-line function call must be the
    |       | last content on the line
 23 | ERROR | Closing parenthesis of a multi-line function call must be on a
    |       | line by itself
@ashnazg
@alcalyn

Yes, putting single quotes in their own lines solves the problem, but I find it ugly :p
Psr2 prefers:

return $this->_em->createQuery(
    '
        select count(p.id)
        from CoreBundle:Player p
        where p.pseudo = :pseudo
        and p.invited = 0
    '
)
->setParameters(array(
    'pseudo' => $pseudo,
))
->getSingleScalarResult();
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.