Merge pull request #127 from green-code-initiative/ISSUE_73_GCI35

dedece35 · web-flow · commit df3b2a2e554d · 2026-06-12T23:31:36.000+02:00
Issue 73 gci35
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -15,7 +15,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - reduce noise on resource unit test files
 - [#111](https://github.com/green-code-initiative/creedengo-php/issues/111) refacto to have all the test files in the same place (for UT and IT), to avoid maintaining 2 test directories
 - [#111](https://github.com/green-code-initiative/creedengo-php/issues/111) refacto all test files to add sub-directories for each rule, to be more clear and to be able to add more tests for each rule in the future
-- [#126](https://github.com/green-code-initiative/creedengo-php/pull/126) Fix NPE on GCI2
+- [#126](https://github.com/green-code-initiative/creedengo-php/pull/126) Fix NPE on GCI2 and GCI35 + global NPE robustness for these 2 rules
 
 ### Deleted
 
diff --git a/src/it/test-projects/creedengo-php-plugin-test-project/src/GCI2/AvoidMultipleIfElseStatement.php b/src/it/test-projects/creedengo-php-plugin-test-project/src/GCI2/AvoidMultipleIfElseStatement.php
@@ -451,7 +451,7 @@ public function shouldBeNotCompliantBecauseTheSameVariableIsUsedManyTimes() // N
     }
 
     // COMPLIANT
-    // previous NPE error in else case
+    // (previously NPE error in else case)
     public function median(array $numbers)
     {
         sort($numbers);
diff --git a/src/it/test-projects/creedengo-php-plugin-test-project/src/GCI2/AvoidMultipleIfElseStatementEdgeCases.php b/src/it/test-projects/creedengo-php-plugin-test-project/src/GCI2/AvoidMultipleIfElseStatementEdgeCases.php
@@ -0,0 +1,42 @@
+<?php
+
+interface StatsContract
+{
+    // No method body: covers METHOD_DECLARATION without BLOCK.
+    public function compute(array $values);
+}
+
+abstract class BaseStats
+{
+    // No method body: covers METHOD_DECLARATION without BLOCK in abstract class.
+    abstract protected function normalize(array $values);
+}
+
+class AvoidMultipleIfElseStatementEdgeCases extends BaseStats implements StatsContract
+{
+    public function compute(array $values)
+    {
+        // Early return keeps fixture simple and avoids duplicating main conditional chains.
+        if (empty($values)) {
+            return 0;
+        }
+
+        $isReady = count($values) > 1;
+
+        // Core complementary scenario for GCI2: non-binary IF condition.
+        // No variable is collected from condition, ELSE traversal must remain safe (no NPE).
+        if ($isReady) {
+            return $values[0];
+        } else {
+            return -1;
+        }
+    }
+
+    protected function normalize(array $values)
+    {
+        // Keep this method intentionally branch-free to avoid duplicating tested IF patterns.
+        return $values;
+    }
+}
+
+
diff --git a/src/it/test-projects/creedengo-php-plugin-test-project/src/GCI35/AvoidTryCatchWithFileOpenedCheck.php b/src/it/test-projects/creedengo-php-plugin-test-project/src/GCI35/AvoidTryCatchWithFileOpenedCheck.php
@@ -160,3 +160,21 @@
 } catch (\Exception $e) {
     echo $e->getMessage() . ' catch in\n';
 }
+
+// COMPLIANT
+// (previously NPE)
+function vq_cache_set_page(string $identifiant, string $content, string $slug): void {
+    if(!is_user_logged_in())
+    {
+        global $wpdb;
+
+        try
+        {
+            $wpdb->replace('commune_content', array('code_insee'=>$identifiant, 'content'=>$content, 'slug'=>$slug));
+        }
+        catch(Exception $e)
+        {
+            vq_log(level: 'warning', filename: basename(__FILE__), message: 'Failed to store commune page content in cache. ' . $e->getMessage());
+        }
+    }
+}
diff --git a/src/it/test-projects/creedengo-php-plugin-test-project/src/GCI35/AvoidTryCatchWithFileOpenedCheckEdgeCases.php b/src/it/test-projects/creedengo-php-plugin-test-project/src/GCI35/AvoidTryCatchWithFileOpenedCheckEdgeCases.php
@@ -0,0 +1,53 @@
+<?php
+
+class AvoidTryCatchWithFileOpenedCheckEdgeCases
+{
+    public function shouldCoverCompliantAndNonCompliantBranches(bool $flag): void
+    {
+        try {
+            echo strtoupper('hello');
+            echo 42;
+        } catch (\Exception $e) {
+            echo $e->getMessage();
+        }
+
+        try {
+            $callable = 'fopen';
+            echo $callable('myfile.txt', 'r');
+        } catch (\Exception $e) {
+            echo $e->getMessage();
+        }
+
+        try {
+            fOpen('myfile.txt', 'r'); // NOK {{Avoid the use of try-catch with a file open in try block}}
+        } catch (\Exception $e) {
+            echo $e->getMessage();
+        }
+
+        try {
+            // Function call not in the banned list: should stay compliant.
+            parse_url('https://green-code-initiative.org');
+        } catch (\Exception $e) {
+            echo $e->getMessage();
+        }
+
+        try {
+            // Assignment whose value is not a function call: should be ignored by the rule.
+            $result = 1;
+            echo $result;
+        } catch (\Exception $e) {
+            echo $e->getMessage();
+        }
+
+        try {
+            if ($flag) {
+                // RETURN_STATEMENT is intentionally not analyzed by GCI35 and should be ignored.
+                return;
+            }
+        } catch (\Exception $e) {
+            echo $e->getMessage();
+        }
+    }
+}
+
+
diff --git a/src/main/java/fr/greencodeinitiative/php/checks/GCI2AvoidMultipleIfElseStatementCheck.java b/src/main/java/fr/greencodeinitiative/php/checks/GCI2AvoidMultipleIfElseStatementCheck.java
@@ -31,6 +31,7 @@
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Collections;
 
 /**
  * FUNCTIONAL DESCRIPTION : please see ASCIIDOC description file of this rule (inside `creedengo-rules-spcifications`)
@@ -259,8 +260,6 @@ private void computeElseVariables(ElseClauseTree pElseTree, int pLevel) {
 
         Map<String, Integer> currentIfVars = variablesStruct.getVariablesForCurrentIfStruct(pLevel);
 
-        if (currentIfVars == null) { return; }
-
         for (Map.Entry<String, Integer> entry : currentIfVars.entrySet()) {
             String variableName = entry.getKey();
 
@@ -338,7 +337,9 @@ private Integer internalGetVariableUsageOfNearestParent(Map<Integer, Map<String,
             Integer nbParentUsed = null;
             for (int i = pLevel; i >= 0 && nbParentUsed == null; i--) {
                 Map<String, Integer> variablesParentLevelMap = pDataMap.get(i);
-                nbParentUsed = variablesParentLevelMap.get(variableName);
+                if (variablesParentLevelMap != null) {
+                    nbParentUsed = variablesParentLevelMap.get(variableName);
+                }
             }
 
             return nbParentUsed;
@@ -355,12 +356,7 @@ public void reinitVariableUsageForLevel(int pLevel) {
          * reinitialization of variable usages in input level in input map
          */
         private void internalReinitVariableUsageForLevelForCurrentIfStruct(Map<Integer, Map<String, Integer>> pDataMap, int pLevel) {
-            if (pDataMap.get(pLevel) == null) { return; }
-
-            // cleaning of current If Structure beginning at level specified
-            for (int i = pLevel; i < pDataMap.size(); i++) {
-                pDataMap.remove(i);
-            }
+            pDataMap.keySet().removeIf(level -> level >= pLevel);
 
         }
 
@@ -382,7 +378,7 @@ public void incrementVariableUsageForLevelForCurrentIfStruct(String variableName
          * get usage of a variable in a level on if/elseif map
          */
         public Map<String, Integer> getVariablesForCurrentIfStruct(int pLevel) {
-            return mapVariablesPerLevelForCurrentIfStruct.get(pLevel);
+            return mapVariablesPerLevelForCurrentIfStruct.getOrDefault(pLevel, Collections.emptyMap());
         }
 
     }
diff --git a/src/main/java/fr/greencodeinitiative/php/checks/GCI35AvoidTryCatchWithFileOpenedCheck.java b/src/main/java/fr/greencodeinitiative/php/checks/GCI35AvoidTryCatchWithFileOpenedCheck.java
@@ -45,65 +45,105 @@ public List<Tree.Kind> nodesToVisit() {
     }
 
     @Override
-    public void visitNode(Tree tree) {
+    public void visitNode(@SuppressWarnings("NullableProblems") Tree tree) {
 
-        TryStatementTree tryStatement = (TryStatementTree) tree;
+        if (!(tree instanceof TryStatementTree tryStatement)) {
+            return;
+        }
 
         // parcours des statements du bloc try
-        visitStatementsList(tryStatement.block().statements());
+        if (tryStatement.block() != null) {
+            visitStatementsList(tryStatement.block().statements());
+        }
 
     }
 
     private void visitStatementsList(List<StatementTree> lstStmts) {
+        if (lstStmts == null || lstStmts.isEmpty()) {
+            return;
+        }
         for (StatementTree stmt : lstStmts){
+            if (stmt == null) {
+                continue;
+            }
             Tree.Kind kind = stmt.getKind();
             switch (kind) {
                 case EXPRESSION_STATEMENT -> visitExpressionStatement(((ExpressionStatementTree) stmt).expression());
-                case BLOCK -> visitStatementsList(((BlockTree) stmt).statements());
+                case BLOCK -> visitIfNotNullStatements(((BlockTree) stmt).statements());
                 case IF_STATEMENT, ALTERNATIVE_IF_STATEMENT -> visitIfStatement((IfStatementTree) stmt);
-                case FOR_STATEMENT, ALTERNATIVE_FOR_STATEMENT -> visitStatementsList(((ForStatementTree) stmt).statements());
-                case WHILE_STATEMENT, ALTERNATIVE_WHILE_STATEMENT -> visitStatementsList(((WhileStatementTree) stmt).statements());
+                case FOR_STATEMENT, ALTERNATIVE_FOR_STATEMENT -> visitIfNotNullStatements(((ForStatementTree) stmt).statements());
+                case WHILE_STATEMENT, ALTERNATIVE_WHILE_STATEMENT -> visitIfNotNullStatements(((WhileStatementTree) stmt).statements());
                 case DO_WHILE_STATEMENT -> visitStatementsList(Collections.singletonList(((DoWhileStatementTree) stmt).statement()));
-                case FOREACH_STATEMENT, ALTERNATIVE_FOREACH_STATEMENT -> visitStatementsList(((ForEachStatementTree) stmt).statements());
-                case CASE_CLAUSE -> visitStatementsList(((CaseClauseTree) stmt).statements());
+                case FOREACH_STATEMENT, ALTERNATIVE_FOREACH_STATEMENT -> visitIfNotNullStatements(((ForEachStatementTree) stmt).statements());
+                case CASE_CLAUSE -> visitIfNotNullStatements(((CaseClauseTree) stmt).statements());
                 case SWITCH_STATEMENT -> visitSwitchStatement((SwitchStatementTree) stmt);
-                case DEFAULT_CLAUSE -> visitStatementsList(((DefaultClauseTree) stmt).statements());
+                case DEFAULT_CLAUSE -> visitIfNotNullStatements(((DefaultClauseTree) stmt).statements());
                 case TRY_STATEMENT -> visitTryStatement((TryStatementTree) stmt);
+                default -> {
+                    // Nothing to visit for unsupported statement kinds.
+                }
             }
         }
     }
 
+    private void visitIfNotNullStatements(List<StatementTree> statements) {
+        if (statements != null) {
+            visitStatementsList(statements);
+        }
+    }
+
     private void visitIfStatement(IfStatementTree ifStatement) {
+        if (ifStatement == null) {
+            return;
+        }
         visitStatementsList(ifStatement.statements());
 
-        if (ifStatement.elseClause() != null) {
-            visitStatementsList(ifStatement.elseClause().statements());
+        ElseClauseTree elseClause = ifStatement.elseClause();
+        if (elseClause != null) {
+            visitIfNotNullStatements(elseClause.statements());
         }
 
-        if (ifStatement.elseifClauses() != null) {
-            for (ElseifClauseTree stmtElseIf : ifStatement.elseifClauses()) {
-                visitStatementsList(stmtElseIf.statements());
+        List<ElseifClauseTree> elseIfClauses = ifStatement.elseifClauses();
+        if (elseIfClauses != null) {
+            for (ElseifClauseTree stmtElseIf : elseIfClauses) {
+                if (stmtElseIf != null) {
+                    visitIfNotNullStatements(stmtElseIf.statements());
+                }
             }
         }
     }
 
     private void visitTryStatement(TryStatementTree tryStatement) {
+        if (tryStatement == null) {
+            return;
+        }
         for (CatchBlockTree stmtCatch : tryStatement.catchBlocks()) {
-            visitStatementsList(stmtCatch.block().statements());
+            if (stmtCatch.block() != null) {
+                visitIfNotNullStatements(stmtCatch.block().statements());
+            }
         }
 
-        if (tryStatement.finallyBlock() != null) {
-            visitStatementsList(tryStatement.finallyBlock().statements());
+        var finallyBlock = tryStatement.finallyBlock();
+        if (finallyBlock != null) {
+            visitIfNotNullStatements(finallyBlock.statements());
         }
     }
 
     private void visitSwitchStatement(SwitchStatementTree switchStatement) {
+        if (switchStatement == null || switchStatement.cases() == null) {
+            return;
+        }
         for (SwitchCaseClauseTree switchCaseStmt : switchStatement.cases()) {
-            visitStatementsList(switchCaseStmt.statements());
+            if (switchCaseStmt != null && switchCaseStmt.statements() != null) {
+                visitIfNotNullStatements(switchCaseStmt.statements());
+            }
         }
     }
 
     private void visitExpressionStatement(ExpressionTree exprTree){
+        if (exprTree == null) {
+            return;
+        }
         if (exprTree.is(Tree.Kind.FUNCTION_CALL)) { // si directement "function call"
             visitCallExpression((FunctionCallTree) exprTree);
         } else if (exprTree.is(Tree.Kind.ASSIGNMENT)) { // ou si assignment
@@ -124,8 +164,10 @@ private void visitCallExpression(FunctionCallTree functionCall){
     }
 
     private String getFunctionNameFromCallExpression(FunctionCallTree functionCall) {
-        NamespaceNameTree nspTree = (NamespaceNameTree)functionCall.callee();
-        return nspTree != null && nspTree.fullName() != null ? nspTree.fullName() : "";
+        if (functionCall == null || !(functionCall.callee() instanceof NamespaceNameTree nspTree)) {
+            return "";
+        }
+        return nspTree.fullName() != null ? nspTree.fullName() : "";
     }
 
 }
diff --git a/src/test/java/fr/greencodeinitiative/php/checks/GCI2AvoidMultipleIfElseStatementCheckTest.java b/src/test/java/fr/greencodeinitiative/php/checks/GCI2AvoidMultipleIfElseStatementCheckTest.java
@@ -25,9 +25,22 @@
 
 class GCI2AvoidMultipleIfElseStatementCheckTest {
 
+    // All GCI2 fixtures are grouped in this folder to keep each test focused on one use-case family.
+    private static final String TESTFILES_PATH = System.getProperty("testfiles.path") + "/GCI2/";
+
+    @Test
+    void shouldValidateAllMainUseCases() {
+        // Covers the full official catalog of compliant/non-compliant IF/ELSEIF/ELSE chains,
+        // including the historical median() scenario that previously triggered a NPE.
+        PHPCheckTest.check(new GCI2AvoidMultipleIfElseStatementCheck(), new PhpTestFile(new File(TESTFILES_PATH + "AvoidMultipleIfElseStatement.php")));
+    }
+
     @Test
-    void test() {
-        PHPCheckTest.check(new GCI2AvoidMultipleIfElseStatementCheck(), new PhpTestFile(new File(System.getProperty("testfiles.path") + "/GCI2/AvoidMultipleIfElseStatement.php")));
+    void shouldValidateEdgeCasesWithoutNpe() {
+        // Covers complementary defensive paths (without duplicating main fixture use cases):
+        // methods without body, non-binary conditions and safe ELSE traversal when no variable
+        // has been collected for the current IF structure.
+        PHPCheckTest.check(new GCI2AvoidMultipleIfElseStatementCheck(), new PhpTestFile(new File(TESTFILES_PATH + "AvoidMultipleIfElseStatementEdgeCases.php")));
     }
 
 }
diff --git a/src/test/java/fr/greencodeinitiative/php/checks/GCI35AvoidTryCatchWithFileOpenedCheckTest.java b/src/test/java/fr/greencodeinitiative/php/checks/GCI35AvoidTryCatchWithFileOpenedCheckTest.java
@@ -25,9 +25,22 @@
 
 class GCI35AvoidTryCatchWithFileOpenedCheckTest {
 
+    // All GCI35 fixtures are grouped in this folder to keep each test focused on one use-case family.
+    private static final String TESTFILES_PATH = System.getProperty("testfiles.path") + "/GCI35/";
+
+    @Test
+    void shouldValidateAllMainUseCases() {
+        // Covers the official non-compliant catalog (fopen/readfile/PDF_OPEN in nested structures)
+        // and a previously failing compliant scenario around try/catch traversal.
+        PHPCheckTest.check(new GCI35AvoidTryCatchWithFileOpenedCheck(), new PhpTestFile(new File(TESTFILES_PATH + "AvoidTryCatchWithFileOpenedCheck.php")));
+    }
+
     @Test
-    void test() {
-        PHPCheckTest.check(new GCI35AvoidTryCatchWithFileOpenedCheck(), new PhpTestFile(new File(System.getProperty("testfiles.path") + "/GCI35/AvoidTryCatchWithFileOpenedCheck.php")));
+    void shouldValidateEdgeCasesAndDefensiveBranches() {
+        // Covers complementary paths (without duplicating main fixture scenarios):
+        // case-insensitive match, dynamic callable (compliant), non-function assignment and
+        // unsupported statement kinds that must be ignored.
+        PHPCheckTest.check(new GCI35AvoidTryCatchWithFileOpenedCheck(), new PhpTestFile(new File(TESTFILES_PATH + "AvoidTryCatchWithFileOpenedCheckEdgeCases.php")));
     }
 
 }

Original file line number	Diff line number	Diff line change
`@@ -451,7 +451,7 @@ public function shouldBeNotCompliantBecauseTheSameVariableIsUsedManyTimes() // N`
`451`	`451`	`}`
`452`	`452`
`453`	`453`	`// COMPLIANT`
`454`		`- // previous NPE error in else case`
	`454`	`+ // (previously NPE error in else case)`
`455`	`455`	`public function median(array $numbers)`
`456`	`456`	`{`
`457`	`457`	`sort($numbers);`