Include Generic.Arrays.DisallowLongArraySyntax

[andrewnicols]

Disallow the long array syntax array(), preferring the short array syntax ([]) instead.

[andrewnicols]

I'm not sure how we feel about depending on other projects, but I'm currently using the following rules locally too:

    <rule ref="MediaWiki.Arrays.TrailingComma">
      <properties>
        <property name="singleLine" value="false" />
        <property name="multiLine" value="true" />
      </properties>
    </rule>
    <rule ref="Generic.Arrays.ArrayIndent" />

These ensure that:

• single-line arrays have no trailing-comma
• multi-line arrays have a trailing-comma
• arrays are indented appropriately

The MediaWiki Sniff is licensed under GPLv2:
https://packagist.org/packages/mediawiki/mediawiki-codesniffer

[timhunt]

Perhaps, if we think the MediaWiki sniff is generally useful, we should (with their consent) contribute it upstream, renamed to Generic.Arrays.... ?

[stronk7]

Hi,

while there are some good standards out there (say Slevomat Mammoth, specially and also some nice picks from others like Drupal, Mediawiki...) I'm in favour of using exclusively the Sniffs that come with phpcs (Generic, Pear, Squiz, PSRX...) and our very own ones (of course).

The only dependency that we currently have is the PHPCompatibility one and, to be honest, I'm not sure how useful it is because they haven't released since 2019 (and their unreleased master branch has A LOT of improvements, in fact we use it always as a regular issue in the PHP X.Y epics).

Also, in other standards, they can change dependencies suddenly, like having a Mediawiki XXX file or checkout... and other things... so can introduce unexpected behaviours. We already do that for some of our Sniffs (instrospect version.php, load components.json, soon load also apis.json...)

So I'd say, let's copy (adopt, respecting copyrights) but make them ours. Also, triple check that none of the bundled standards have it. Sometimes I've found some good gems, specially within Squiz.

My very basic opinion. Ciao :-)

[timhunt]

I agree with Eloy, but with the addition that, if a third-part ruleset has a useful potentially-generic sniff, then we should try got get that sniff moved to the generic ruleset. (In our copious free time, of course, but while that may take more work in the short term, it probably saves in maintenance in the long term.)

[stronk7]

I also agree with Tim, to propose something (clearly logic and global) to find its place into the upstream standards makes perfect sense.

[stronk7]

While I personally like this (switch to short syntax), here there are a couple of reflexions:

• Maybe we should resolve https://tracker.moodle.org/browse/MDLSITE-4776 (since 2016!) before?
• Depending of the outcome, make this an error (current implementation) or a warning.

Thoughts? Can be a nice Xmas present for everybody out there (for the good and the bad, heh).

Ciao :-)

[andrewnicols]

Let's do it.

[stronk7]

Can be a nice Xmas present for everybody out there (for the good and the bad, heh).

It seems that it will be a NY present, instead of a Xmas one, but yay in any case!

[stronk7]

Hi,

with https://tracker.moodle.org/browse/MDLSITE-4776 decided and documented... it's time to decide about this one. Only point open to discussion is if we should introduce this as a warning, or go straight to be an error (current implementation).

For some reason, I prefer the laterformer, so people can ignore warnings if desired to. We can also create an issue to raise it to error in, say, 1 year from now or something like that.

Thoughts?

[jrchamp]

For all new code, it's definitely an error. For existing code, it's technically a warning. Rather than try to differentiate, it's probably best to have it set as an error in all cases. Anyone who is not being strict about following moodle-cs in their plugins may see some additional "error" messages. I would hope that anyone that is strict about following moodle-cs is willing to update existing code to align with these changes.

Summary: +1 error

[jrchamp]

Soon it will be a Valentine's Day present 😉

[stronk7]

Soon it will be a Valentine's Day present

The question is... of which year? 😛

Seriously, I've asked the team for opinions about to:

• Make this a warning by default.
• Share about it here and there.
• Raise to error in 1 year.

We have followed that approach in the past (for example the "and/or" one was raised few days ago @ #53) and I think it has worked ok enough.

Ciao :-)

[codecov]

Codecov Report

Merging #34 (37033b0) into main (ef04fab) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##               main      #34   +/-   ##
=========================================
  Coverage     95.53%   95.53%           
  Complexity      440      440           
=========================================
  Files            19       19           
  Lines          1165     1165           
=========================================
  Hits           1113     1113           
  Misses           52       52           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[stronk7]

I've rebased your branch on top of current main to force checks to run again.

So the only remaining point is if we apply for this over 1 year or no:

diff --git a/moodle/ruleset.xml b/moodle/ruleset.xml
index e23657c..ac4628d 100644
--- a/moodle/ruleset.xml
+++ b/moodle/ruleset.xml
@@ -9,7 +9,9 @@
     <arg name="extensions" value="php" />
     <arg name="encoding" value="utf-8" />
 
-    <rule ref="Generic.Arrays.DisallowLongArraySyntax"/>
+    <rule ref="Generic.Arrays.DisallowLongArraySyntax">
+        <type>warning</type>
+    </rule>
 
     <rule ref="Generic.Classes.DuplicateClassName"/>
     <rule ref="Generic.Classes.OpeningBraceSameLine"/>

(note that the tests don't need to be amended because they don't use the ruleset.xml file, so they continue being errors there)

From the team, I've got a couple of 👍 towards going the warning => error way. And, also, people can always go to use the new, stricter, standard if wanted to (see #57).

Ciao :-)

[stronk7]

Ok, so the plan is:

• make it a "warning" in the moodle standard.
• make it an "error" in the moodle-extra standard.
• in 1 year (September 2024) make it an error in both standards (Raise Generic.Arrays.DisallowLongArraySyntax to error - September 2024 #58 has been created about that).

On it right now (towards a release)... ciao :-)

[stronk7]

Hi,

I've updated your branch/commit with the details commented above (warning for moodle and error for moodle-extra). Both to become error in 1 year by #58.

Tested locally, it works ok:

$ vendor/bin/phpcs --standard=moodle pp.php 

FILE: /Users/stronk7/git_moodle/moodle-cs/pp.php
------------------------------------------------------------------------------------------------------------
FOUND 2 ERRORS AND 1 WARNING AFFECTING 2 LINES
------------------------------------------------------------------------------------------------------------
...
...
 3 | WARNING | [x] Short array syntax must be used to define arrays
------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
------------------------------------------------------------------------------------------------------------

Time: 157ms; Memory: 14MB

$ vendor/bin/phpcs --standard=moodle-extra pp.php 

FILE: /Users/stronk7/git_moodle/moodle-cs/pp.php
----------------------------------------------------------------------------------------------------------
FOUND 3 ERRORS AFFECTING 2 LINES
----------------------------------------------------------------------------------------------------------
...
...
 3 | ERROR | [x] Short array syntax must be used to define arrays
----------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------------------------------------------

Time: 145ms; Memory: 14MB

Ciao :-)

[stronk7]

Looks good for both Andrew and me, hence, approving.

[stronk7]

Merged, will be making a release with this and the new Moodle-extra standard in 1-2h…

Thanks all, ciao :)