sniff for attributes being multiline

[joachim-n]

Is your feature request related to a problem?

AFAICT there's no sniffs currently for attribute formatting.

It would be useful to have a sniff that detects (and fixes!) attributes not being multiline.

Describe the solution you'd like

For example:

#[MyAttribute(id: 'alpha', label: new TranslatableMarkup('Alpha'), locked: FALSE)]

should be this:

#[MyAttribute(
  id: 'alpha', 
  label: new TranslatableMarkup('Alpha'), 
  locked: FALSE,
)]

Does an attribute creation count as an object creation such as new Foo(1, 2, 3)? And do either of them count as a function call? I can see that FunctionCallSignatureSniff has a method isMultiLineCall(). Would this be the right place to start?

Additional context (optional)

• [x ] I have read the Contribution Guidelines and this is not a support question.
• [?] I intend to create a pull request to implement this feature. -- Not sure I know enough about the internals of PHPCS!

[jrfnl]

HI @joachim-n Thanks for raising this topic. You are right to say that PHPCS itself currently has no sniffs related to attribute formatting, though there may be external standards which do.

Does an attribute creation count as an object creation such as new Foo(1, 2, 3)? And do either of them count as a function call? I can see that FunctionCallSignatureSniff has a method isMultiLineCall(). Would this be the right place to start?

Object creation often does count as a function call, but I suspect projects may not always want to apply the same rules to attributes as to function calls, so I think a separate category Attributes in the Generic standard containing a set of sniffs related to attribute formatting would be the way forward.
That way, projects can elect to include the attribute related sniffs they desire and will not suddenly get new error messages from sniffs they already use.

To determine what sniffs are needed, I believe PER 3.0 would be a good starting point: https://www.php-fig.org/per/coding-style/#12-attributes
That doesn't mean that there should only be sniffs to satisfy PER, but the PER rules can be used as inspiration for what sniffs to write.

I would also advocate for multiple sniffs instead of one huge one which tries to do to much. Smaller sniffs are easier to write and maintain and give people more flexibility when configuring their project rulesets.

Not sure I know enough about the internals of PHPCS!

Detecting attributes is quite straight forward - just sniff for T_ATTRIBUTE (this is the #[ bracket). The close bracket also has its own token T_ATTRIBUTE_END.
Additionally, all tokens within an attribute will have the 'attribute_opener' and 'attribute_closer' indexes set to the attribute brackets.

Does that help ?

Next steps

I think the next step for this would be to determine what sniffs would be needed to allow for formatting attributes in various ways.

Once we have a list, people can "claim" one or more of these to start writing them (like the multi-line formatting one you suggest above).

How does that sound to you ?

[jrfnl]

Hmm... and thinking about this even further... it may even be better if we add (some of) the sniffs to PHPCSExtra instead of PHPCS itself as that would allow for using the utility functions from PHPCSUtils, which should make writing a sniff for the multi-line formatting a lot easier.

[joachim-n]

Thanks for the pointers!

So based on the PER standards, we could have sniffs for:

• No blank line between an attribute and the things before or after it: 'Attributes on classes, methods, functions, constants and properties MUST be placed on their own line, immediately prior to the structure being described. There MUST NOT be any blank lines between the docblock and attributes, or the attributes and the structure. [...] If two separate attribute blocks are used in a multi-line context, they MUST be on separate lines with no blank lines between them.'
• Attribute names MUST immediately follow the opening attribute block indicator #[ with no space.
• If an attribute has no arguments, the () MUST be omitted.
• The closing attribute block indicator ] MUST follow the last character of the attribute name or the closing ) of its argument list, with no preceding space.

The standards also say:

The attribute arguments MUST follow the same rules as defined for multiline function calls.

Is there currently a sniff for multiline function calls?

[jrfnl]

So based on the PER standards, we could have sniffs for: ....

I think the second and fourth bullet can be combined in one sniff. Something like AttributeBracketSpacing and I imagine that should probably have a public spacing property with a 0 default value (to allow people to set it to 1 if they want spacing on the inside of the brackets).

No blank line between an attribute and the things before or after it

I believe the "no blank line before" only applies when the "thing" has a docblock.

There should probably be a separate sniff to enforce that if there is a docblock, attributes are between the docblock and the structure it applies to. Or maybe there should be two sniffs - one to enforce the "docblock - attribute - structure declaration" order and one (lower priority) which enforces "attribute - docblock - structure declaration".

I also think the first bullet needs to take into account that attributes for function parameters have different placement rules, so need to be excluded from that particular rule.
And IIRC, a number of the Commenting/docblock sniffs already handle this in part (at least for some of the "no blank lines between" parts).

The placement rules for attributes on function parameters should probably be added to the existing Squiz.Functions.FunctionDeclarationArgumentSpacing and Squiz.Functions.MultiLineFunctionDeclaration (both in use by PSR12) as I suspect having this in a separate sniff would otherwise lead to fixer conflicts. And I suspect those sniffs are very likely to be broken currently for function declarations with attributes on parameters (though I don't recall seeing any bug reports for this so far).

And then there are rules about the spacing/formatting inside the attribute, like the comma spacing and that multi-line attributes are only allowed to have one attribute per "block".

All in all, I think the list of sniffs needed is larger.

The attribute arguments MUST follow the same rules as defined for multiline function calls.

Is there currently a sniff for multiline function calls?

PSR12 currently applies the following rules/sniffs for function calls, though this list and/or the sniffs may need updating for PER:

PHP_CodeSniffer/src/Standards/PSR12/ruleset.xml

Lines 200 to 211 in ca606d9

	<!-- 4.7 Method and Function Calls -->
	<!-- When making a method or function call, there MUST NOT be a space between the method or function name and the opening parenthesis, there MUST NOT be a space after the opening parenthesis, and there MUST NOT be a space before the closing parenthesis. In the argument list, there MUST NOT be a space before each comma, and there MUST be one space after each comma. -->
	<!-- Argument lists MAY be split across multiple lines, where each subsequent line is indented once. When doing so, the first item in the list MUST be on the next line, and there MUST be only one argument per line. A single argument being split across multiple lines (as might be the case with an anonymous function or array) does not constitute splitting the argument list itself. -->
	<rule ref="Generic.Functions.FunctionCallArgumentSpacing"/>
	<rule ref="PSR2.Methods.FunctionCallSignature"/>
	<rule ref="PSR2.Methods.FunctionCallSignature.SpaceAfterCloseBracket">
	<severity>0</severity>
	</rule>
	<rule ref="PSR2.Methods.FunctionCallSignature.OpeningIndent">
	<severity>0</severity>
	</rule>

Would be great if there was some progress on the PER project (#29), which should give us more insight into what's needed in that respect, but I'm sorry to say that I have the impression that not much is happening there, despite a number of people volunteering.

If it helps, I'd be happy to do a pairing session to help get you started (once PHPCS 4.0 has been released) or to brainstorm this out some more.

[jrfnl]

Just thinking - we'd also need to verify if the ScopeIndent sniff handles indentation of attributes - to align with the structure it applies to if the attribute is on it's own line - correctly. If not, this may also need its own sniff.

[jrfnl]

Would be great if there was some progress on the PER project (#29), which should give us more insight into what's needed in that respect, but I'm sorry to say that I have the impression that not much is happening there, despite a number of people volunteering.

@joachim-n @fredden and me just spend some time dissecting the PER rules into an action list for sniffs which would need to be written and I've also been working on some additional utility methods for attributes for PHPCSUtils to help with this (see PHPCSUtils#616). I expect to be able to pull these utilities later this week.

Now looking at what we came up with based on the PER rules, I don't actually see any rule in PER about attribute instantiation with multiple/named parameters needing to be multi-line.

So, if anyone were to write a sniff for this, we'd first need to define when an attribute instantiation should be multi-line. What are the criteria ?

Got any suggestions for that ?

In other words, the "ask" with which you originally opened this issue will need to be made more specific before this ticket becomes actionable.

---

Regarding the list of sniffs to satisfy PER which @fredden and me have now come up with: I intend to open issues (probably mostly in PHPCSExtra) for each new sniff which will need to be written.
I will also open issues in this repo about pre-existing sniffs which will need to be reviewed and possible adjusted to address PER.

I will finish the initial work in PHPCSUtils first though, as that will inform which sniffs need to go into PHPCSExtra and which could possibly go into PHP_CodeSniffer itself.

[joachim-n]

Drupal plugins use attributes on classes, which are multiple, and indeed have arrays within them as multiline too, e.g.

#[ConfigEntityType(
  id: 'node_type',
  label: new TranslatableMarkup('Content type'),
  label_collection: new TranslatableMarkup('Content types'),
  label_singular: new TranslatableMarkup('content type'),
  label_plural: new TranslatableMarkup('content types'),
  config_prefix: 'type',
  entity_keys: [
    'id' => 'type',
    'label' => 'name',
  ],
SNIP

There's no formal standard for that yet, but the pattern is that it's only online if there only one value and no key:

#[ViewsArea("result")]

I was coming to this from work in the Drupal space to convert plugins from Doctrine annotations to Plugins. We have a PHP Rector script for that, but it produces the converted annotation as one long line, so I was wondering if PHPCBF could clean it up!

[jrfnl]

@joachim-n Okay, I get where you are coming from now.

Thinking this over some more, my intuition is telling me that kind of criteria/logic should not be mixed into a generic function call formatting sniff.

What I mean by that, is that there could (should) be a separate sniff, or even several sniffs, to analyse whether a function call should be single-line or multi-line.
I can imagine different projects may have very different criteria:

• Based on number of arguments passed to the function call.
• Based on whether those are named arguments or not.
• Based on line length.
• [Edit/added] Based on whether the function call is actually a class instantiation/attribute or not.
• etc.

Obviously, these criteria are highly arbitrary and should probably be configurable (with some reasonable default values).

The sniff(s) would then only really "fix" the function call from single-line to multi-line, after which a generic function call formatting sniff can kick in to clean up the multi-line formatting.

Advantages of that approach would be:

• Flexibility for projects to choose the criteria and possibly combine multiple criteria.
• Less/No risk of fixer conflicts, as these sniffs would just "force" the function call from single line to multi-line (and possibly vice versa too), while leaving the function call formatting (indentation and other spacing and such) to whatever sniff a project has selected for that.

How does that sound to you ?

As the sniff(s) would have highly arbitrary criteria, I'm inclined to say it should go into PHPCSExtra instead of PHPCS itself. That would also make writing it much more straight-forward as PHPCSUtils has methods available for parsing function call arguments, counting them etc,

If the above sounds like a good way forward to you, I'll transfer the issue to PHPCSExtra.