Allow to configure PHPMD with php or yaml file #1058

kylekatarnls · 2024-01-07T12:18:47Z

Type: feature
Breaking change: yes

kylekatarnls · 2024-01-07T15:38:43Z

Note: the src/conf/phpmd.schema.json file is what allow auto-completion in IDEs:

To be automatically applied to phpmd.yml files by IDE, we'll need to submit it in
https://github.com/SchemaStore/schemastore/tree/master/src/schemas/json

Then to be associated to relevant file names in the catalog:
https://github.com/SchemaStore/schemastore/blob/master/src/api/json/catalog.json

kylekatarnls · 2024-01-07T16:49:21Z

src/test/resources/files/rulesets/phpmd.yml is an example of how it would look to configure rule with a YAML file.

It would work the same by providing JSON file or PHP file returning the same array structure.

kylekatarnls · 2024-01-07T16:51:14Z

src/test/php/PHPMD/TextUI/CommandLineOptionsTest.php

+        $options = new CommandLineOptions([__FILE__, 'app', 'sarif']);

-        $args = [__FILE__, 'text', 'design'];
-        new CommandLineOptions($args);
+        self::assertSame('cleancode,codesize,controversial,design,naming,unusedcode', $options->getRuleSets());
+        self::assertSame('sarif', $options->getReportFormat());
+        self::assertSame('app', $options->getInputPath());
+
+        $options = new CommandLineOptions([__FILE__, 'app']);
+
+        self::assertSame('cleancode,codesize,controversial,design,naming,unusedcode', $options->getRuleSets());
+        self::assertSame('text', $options->getReportFormat());
+        self::assertSame('app', $options->getInputPath());
+
+        $options = new CommandLineOptions([__FILE__]);
+
+        self::assertSame('cleancode,codesize,controversial,design,naming,unusedcode', $options->getRuleSets());
+        self::assertSame('text', $options->getReportFormat());
+        self::assertSame('src', $options->getInputPath());


Arguments would all become optional:
See the default values on https://github.com/phpmd/phpmd/pull/1058/files#diff-d7bddc86dbcab23cca1d846c4d63a3957de2b6d196f6e4527e18161ff908118b

kylekatarnls · 2024-01-07T16:53:24Z

src/main/php/PHPMD/TextUI/CommandLineOptions.php

+            0 => [file_exists('src') ? 'src' : '.', 'text', $this->getDefaultConfig()],
+            1 => ['text', $this->getDefaultConfig()],
+            2 => [$this->getDefaultConfig()],
+            default => [],


input path would fallback to "src" if it's exist, "." else

report format would fallback to "text"

ruleset would fallback to first config file found by getDefaultConfig, or "all" if no file provided.

kylekatarnls · 2024-01-07T16:54:15Z

src/main/php/PHPMD/TextUI/CommandLineOptions.php


        $validator = new ArgumentsValidator($hasImplicitArguments, $originalArguments, $arguments);

-        $this->ruleSets = (string)array_pop($arguments);
+        $ruleSets = (string)array_pop($arguments);
+        $this->ruleSets = $ruleSets === 'all' ? InternalRuleSet::getNamesConcatenated() : $ruleSets;


"all" would be a shorthand for all internal rulesets from PHPMD:

cleancode,codesize,controversial,design,naming,unusedcode

AJenbo · 2024-05-11T16:20:17Z

Not using XML is a big improvement in my book. I prefer the PHP option personally.

@kylekatarnls I think now is a good point to rebase this so it's ready for merging.

codecov · 2024-05-16T17:46:43Z

Codecov Report

Attention: Patch coverage is 93.08176% with 11 lines in your changes missing coverage. Please review.

Project coverage is 91.75%. Comparing base (3aef909) to head (c9dd2f4).
Report is 37 commits behind head on 3.x.

Files with missing lines	Patch %	Lines
src/main/php/PHPMD/RuleSetFactory.php	92.00%	10 Missing ⚠️
src/main/php/PHPMD/TextUI/CommandLineOptions.php	95.23%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##                3.x    #1058   +/-   ##
=========================================
  Coverage     91.75%   91.75%           
- Complexity     1275     1297   +22     
=========================================
  Files           107      109    +2     
  Lines          3370     3443   +73     
=========================================
+ Hits           3092     3159   +67     
- Misses          278      284    +6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/main/php/PHPMD/AbstractNode.php

AJenbo · 2024-05-17T01:21:30Z

src/main/php/PHPMD/RuleSetFactory.php

@@ -32,31 +36,18 @@ class RuleSetFactory
    /**


This calls is getting a bit heavy on pulling out mixed variables and casting them to various types. This is likely going to trigger a lot of unsafe operations on higher PHPStan levels.

AJenbo · 2024-05-17T01:23:06Z

Lots of things going on in one PR, most of it is nice, but a bit hard to review and comment on individual things with it all there.

kylekatarnls · 2024-05-17T09:03:53Z

In theory bcmul can go even above 64 bits, but I wonder if it has a real interest in practice for npath (and also why it would be the only one using big number). BTW it also possible to go above by using float (which is automatic in PHP), it just loose a bit of precision but I'm not sure it's an actual problem, if you have billion npath, you probably care about order of magnitude, not an exact value.

AJenbo · 2024-05-17T10:09:04Z

i wold normally just use it if decimal fractional rounding is important, I have a hard time imagining the need for values that high.

kylekatarnls · 2024-05-18T16:59:03Z

Lots of things going on in one PR, most of it is nice, but a bit hard to review and comment on individual things with it all there.

I'm also unsatisfied about how it growth after re-basing. I will split.

kylekatarnls · 2024-09-08T21:24:17Z

src/main/php/PHPMD/RuleSetFactory.php

@@ -109,8 +112,6 @@ public function createRuleSets(string $ruleSetFileNames): array
     * Creates a single rule-set instance for the given filename or identifier.
     *
     * @param string $ruleSetOrFileName The rule-set filename or identifier.
-     * @throws RuleSetNotFoundException
-     * @throws RuntimeException
     */


Likely just lost in rebasing

Suggested change

*/

* @throws RuleSetNotFoundException

* @throws RuntimeException

*/

kylekatarnls · 2024-09-08T21:25:03Z

src/main/php/PHPMD/RuleSetFactory.php

-                $fileName = $file;
-            } elseif (is_readable($ruleSetFolderPath . DIRECTORY_SEPARATOR . $file)) {
-                $fileName = $ruleSetFolderPath . DIRECTORY_SEPARATOR . $file;
+            $ruleNodeFile = (string )$ruleNode['file'];


Suggested change

$ruleNodeFile = (string )$ruleNode['file'];

$ruleNodeFile = (string) $ruleNode['file'];

src/main/php/PHPMD/RuleSetFactory.php

src/main/php/PHPMD/Node/ASTNode.php

kylekatarnls · 2024-09-08T22:41:20Z

Rebased, I also extracted PHPStan in a dedicated job. Few things are broken here after the rebase, I'll have to check.

AJenbo · 2024-09-08T23:44:10Z

Some of the reported issues are addressed by #1202

AJenbo · 2024-09-12T16:35:10Z

src/main/php/PHPMD/AbstractNode.php

@@ -56,7 +56,9 @@ public function __construct(
     * The magic call method is used to pipe requests from rules direct
     * to the underlying PDepend AST node.
     *
-     * @param array<mixed> $args
+     * @param list<mixed> $args
+     * @throws BadMethodCallException When the underlying PDepend node


Since it's not possible for the analyzer to figure out what function can throw this exception you can use an overwrite to indicate what line can throw it:

/** @throws BadMethodCallException */ return $this->node->{$name}(...$args);

tvbeek

Nice job 👍

We need to have an update for the documentation but I suggest it can be a good idea to create a new PR for that.

tvbeek · 2025-01-10T10:38:39Z

src/main/php/PHPMD/RuleSetFactory.php

    {
-        foreach ($ruleSetNode->exclude as $exclude) {
-            if ($rule->getName() === (string) $exclude['name']) {
+        $excludes = (is_object($ruleSetNode) ? ($ruleSetNode->exclude ?? null) : null)


I'm thinking if it isn't an improvement to create a new method to get the excludes? I see that I need to read it multiple times to see if it is correct.

But that can be a personal style.

tvbeek · 2025-01-10T10:51:14Z

composer.json

@@ -39,7 +39,8 @@
    "php": "^8.1",
    "ext-xml": "*",
    "composer/xdebug-handler": "^3.0",
-    "pdepend/pdepend": "3.x-dev#c21f168eccda0175040c9425b9b7c330176d60c4"
+    "pdepend/pdepend": "3.x-dev#c21f168eccda0175040c9425b9b7c330176d60c4",
+    "symfony/yaml": "^5.4.31 || ^6.0 || ^7.0"


I'm thinking, this is only needed if you use a yaml file. Is it an option to add this to the suggest and check the existence of this package(or the class) in the function that use it?

On that way we lower the number of dependencies that isn't needed for all users.

It depends if we plan to recommend it as the default in our documentation.

kylekatarnls added this to the 3.0.0 milestone Jan 7, 2024

kylekatarnls force-pushed the feature/multi-format-support branch from 67d9440 to 2bedff3 Compare January 7, 2024 14:47

kylekatarnls added Feature Needs Tests Needs Documentation RFC labels Jan 7, 2024

kylekatarnls commented Jan 7, 2024

View reviewed changes

kylekatarnls force-pushed the feature/multi-format-support branch 2 times, most recently from dabeb46 to 51c1a92 Compare January 7, 2024 17:06

kylekatarnls force-pushed the feature/3.x-experiment branch from 799e1db to 5173ebb Compare January 17, 2024 13:27

kylekatarnls force-pushed the feature/multi-format-support branch 2 times, most recently from 0439441 to 7775855 Compare January 17, 2024 15:35

Base automatically changed from feature/3.x-experiment to 3.x April 29, 2024 15:24

kylekatarnls changed the title ~~[Tentative] Allow to configure PHPMD with php or yaml file~~ Allow to configure PHPMD with php or yaml file May 16, 2024

kylekatarnls force-pushed the feature/multi-format-support branch 3 times, most recently from c7d924c to 2be8831 Compare May 16, 2024 17:45

AJenbo reviewed May 16, 2024

View reviewed changes

src/main/php/PHPMD/AbstractNode.php Outdated Show resolved Hide resolved

kylekatarnls force-pushed the feature/multi-format-support branch 4 times, most recently from df1b444 to 2585b4e Compare May 17, 2024 00:04

kylekatarnls marked this pull request as ready for review May 17, 2024 00:17

kylekatarnls requested a review from AJenbo May 17, 2024 00:17

kylekatarnls requested review from ravage84 and tvbeek May 17, 2024 00:17

AJenbo previously approved these changes May 17, 2024

View reviewed changes

ravage84 added the Configuration label May 24, 2024

ravage84 mentioned this pull request May 24, 2024

[3.x] [RFC] Support for New Configuration Formats #1167

Open

kylekatarnls dismissed AJenbo’s stale review via a43bcb3 September 8, 2024 20:36

kylekatarnls force-pushed the feature/multi-format-support branch from 2585b4e to a43bcb3 Compare September 8, 2024 20:36

kylekatarnls mentioned this pull request Sep 8, 2024

[Tentative] Create attributes to represent rule properties modifiable by config #1059

Draft

kylekatarnls commented Sep 8, 2024

View reviewed changes

src/main/php/PHPMD/RuleSetFactory.php Outdated Show resolved Hide resolved

kylekatarnls commented Sep 8, 2024

View reviewed changes

src/main/php/PHPMD/RuleSetFactory.php Outdated Show resolved Hide resolved

kylekatarnls commented Sep 8, 2024

View reviewed changes

src/main/php/PHPMD/Node/ASTNode.php Outdated Show resolved Hide resolved

kylekatarnls force-pushed the feature/multi-format-support branch 5 times, most recently from c88ef0c to e8a5290 Compare September 8, 2024 22:26

Allow to configure PHPMD with php or yaml file

c9dd2f4

kylekatarnls force-pushed the feature/multi-format-support branch from 6f9319c to c9dd2f4 Compare September 11, 2024 08:31

AJenbo reviewed Oct 5, 2024

View reviewed changes

tvbeek reviewed Jan 10, 2025

View reviewed changes

	$ruleNodeFile = (string )$ruleNode['file'];
	$ruleNodeFile = (string) $ruleNode['file'];

Uh oh!

Allow to configure PHPMD with php or yaml file #1058

Are you sure you want to change the base?

Allow to configure PHPMD with php or yaml file #1058

Uh oh!

Conversation

kylekatarnls commented Jan 7, 2024

Uh oh!

kylekatarnls commented Jan 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kylekatarnls commented Jan 7, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AJenbo commented May 11, 2024

Uh oh!

codecov bot commented May 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

AJenbo May 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AJenbo commented May 17, 2024

Uh oh!

kylekatarnls commented May 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AJenbo commented May 17, 2024

Uh oh!

kylekatarnls commented May 18, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kylekatarnls commented Sep 8, 2024

Uh oh!

AJenbo commented Sep 8, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tvbeek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kylekatarnls commented Jan 7, 2024 •

edited

Loading

codecov bot commented May 16, 2024 •

edited

Loading

AJenbo May 17, 2024 •

edited

Loading

kylekatarnls commented May 17, 2024 •

edited

Loading