fix: follow-ups from the 3.10.2 review #519
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -64,6 +64,13 @@ class Validator { | |
| */ | ||
| private array $claimed_identifiers = []; | ||
|
|
||
| /** | ||
| * Namespace the code being read currently declares, lower-cased, or empty for the global namespace. | ||
| * | ||
| * @var string | ||
| */ | ||
| private string $namespace = ''; | ||
|
|
||
| /** | ||
| * Class constructor. | ||
| * | ||
|
|
@@ -130,15 +137,18 @@ private function next() { | |
| */ | ||
| private function check_duplicate_identifier( string $type, string $identifier ): bool { | ||
| $identifier = strtolower( ltrim( $identifier, '\\' ) ); | ||
|
|
||
| // PHP keeps declared names fully qualified, so that is the form compared | ||
| // and claimed: the same short name in two namespaces is two names. | ||
| $qualified = '' === $this->namespace ? $identifier : $this->namespace . '\\' . $identifier; | ||
| $namespaced_identifier = 'code_snippets\\' . $identifier; | ||
|
|
||
| if ( ! isset( $this->defined_identifiers[ $type ] ) ) { | ||
| switch ( $type ) { | ||
| case T_FUNCTION: | ||
| $this->defined_identifiers[ T_FUNCTION ] = array_map( | ||
| 'strtolower', | ||
| array_merge( get_defined_functions()['internal'], get_defined_functions()['user'] ) | ||
| ); | ||
| break; | ||
|
|
||
|
|
@@ -160,18 +170,67 @@ private function check_duplicate_identifier( string $type, string $identifier ): | |
| $this->claimed_identifiers[ $type ] ?? [] | ||
| ); | ||
|
|
||
| $duplicate_identifier = in_array( $qualified, $known, true ); | ||
| $duplicate_namespaced = '' === $this->namespace && in_array( $namespaced_identifier, $known, true ); | ||
| $exceptions = $this->exceptions[ $type ] ?? []; | ||
| $exception_identifier = in_array( $identifier, $exceptions, true ) || in_array( $qualified, $exceptions, true ); | ||
| $exception_namespaced = in_array( $identifier, $exceptions, true ) || in_array( $namespaced_identifier, $exceptions, true ); | ||
|
Comment on lines
+176
to
+177
There was a problem hiding this comment. 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win 🔎 Supported by static analysis🌐 Web query:
💡 Result: No, calling function_exists('shared') inside the namespace Acme does not check for the global function 'shared' [1][2][3]. According to the official PHP documentation, the function_exists function checks for the existence of a function by its fully qualified name [1][3][4]. When you pass a string that does not contain a namespace separator (such as 'shared') to function_exists, it looks specifically for a function named 'shared' in the global namespace [1][3]. It does not perform any namespace-based resolution or fallback, regardless of the namespace in which the call is made [1][3]. This behavior is different from how standard function calls work in PHP; when you call an unqualified function (e.g., shared) inside a namespace, PHP's engine attempts to resolve it by first looking in the current namespace and then falling back to the global namespace [5][6][7]. However, function_exists interprets its string argument literally as a fully qualified name [1][3][4]. To check for a function within a specific namespace, you must provide the full, qualified name (e.g., function_exists('Acme\shared')) [1][3][8]. Citations:
🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- src/php/Utils/Validator.php:130-265 ---'
sed -n '130,265p' src/php/Utils/Validator.php
printf '%s\n' '--- related validator tests ---'
fd -i 'Validator' . | head -20
rg -n -S 'exception_identifier|exception_namespaced|function_exists|class_exists|namespace' tests src/php -g '*.php' | head -160Repository: codesnippetspro/code-snippets Length of output: 21125 🤖 get_repo_knowledge executed:
Length of output: 1954 🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- src/php/Utils/Validator.php:1-130 and 255-330 ---'
sed -n '1,130p' src/php/Utils/Validator.php
sed -n '255,330p' src/php/Utils/Validator.php
printf '%s\n' '--- tests/unit/Utils/Validator_Test.php ---'
cat -n tests/unit/Utils/Validator_Test.phpRepository: codesnippetspro/code-snippets Length of output: 9265 Match unqualified existence checks only in the global namespace. At Require the qualified exception in a named namespace. Keep the short-name exception only in the global namespace. Add PHPUnit coverage. 🧰 Tools🪛 PHPMD (2.15.0)[warning] 10-378: The class Validator has an overall complexity of 73 which is very high. The configured complexity threshold is 50. (undefined) (ExcessiveClassComplexity) [error] 10-378: The property $defined_identifiers is not named in camelCase. (undefined) (CamelCasePropertyName) [error] 10-378: The property $claimed_identifiers is not named in camelCase. (undefined) (CamelCasePropertyName) [warning] 138-183: The method check_duplicate_identifier() has a Cyclomatic Complexity of 12. The configured cyclomatic complexity threshold is 10. (undefined) (CyclomaticComplexity) [error] 138-183: The method check_duplicate_identifier is not named in camelCase. (undefined) (CamelCaseMethodName) 🤖 Prompt for AI AgentsSource: Coding guidelines |
||
|
|
||
| array_unshift( $this->defined_identifiers[ $type ], $qualified ); | ||
| $this->claimed_identifiers[ $type ][] = $qualified; | ||
|
|
||
| return ( $duplicate_identifier && ! $exception_identifier ) || ( $duplicate_namespaced && ! $exception_namespaced ); | ||
| } | ||
|
|
||
| /** | ||
| * Read the name a namespace declaration introduces, leaving the cursor after it. | ||
| * | ||
| * A bare "namespace {" opens the global namespace; "namespace\\foo()" is a | ||
| * relative name rather than a declaration and is left alone. | ||
| * | ||
| * @return string Lower-cased namespace, or empty for the global namespace. | ||
| */ | ||
| private function read_namespace_declaration(): string { | ||
| $name = ''; | ||
|
|
||
| while ( ! $this->end() ) { | ||
| $token = $this->peek(); | ||
|
|
||
| if ( is_array( $token ) ) { | ||
| if ( T_WHITESPACE === $token[0] || T_COMMENT === $token[0] || T_DOC_COMMENT === $token[0] ) { | ||
| $this->next(); | ||
| continue; | ||
| } | ||
|
|
||
| if ( T_NS_SEPARATOR === $token[0] && '' === $name ) { | ||
| return $this->namespace; | ||
| } | ||
|
|
||
| if ( defined( 'T_NAME_RELATIVE' ) && T_NAME_RELATIVE === $token[0] ) { | ||
| return $this->namespace; | ||
| } | ||
|
|
||
| if ( T_STRING === $token[0] || T_NS_SEPARATOR === $token[0] | ||
| || ( defined( 'T_NAME_QUALIFIED' ) && T_NAME_QUALIFIED === $token[0] ) ) { | ||
| $name .= $token[1]; | ||
| $this->next(); | ||
| continue; | ||
| } | ||
|
|
||
| return $this->namespace; | ||
| } | ||
|
|
||
| if ( ';' === $token || '{' === $token ) { | ||
| $this->next(); | ||
| return strtolower( trim( $name, '\\' ) ); | ||
| } | ||
|
|
||
| return $this->namespace; | ||
| } | ||
|
|
||
| return strtolower( trim( $name, '\\' ) ); | ||
| } | ||
|
|
||
| /** | ||
| * Validate the given PHP code and return the result. | ||
| * | ||
|
|
@@ -187,6 +246,11 @@ public function validate() { | |
| continue; | ||
| } | ||
|
|
||
| if ( T_NAMESPACE === $token[0] ) { | ||
| $this->namespace = $this->read_namespace_declaration(); | ||
| continue; | ||
| } | ||
|
|
||
| // If this is a function or class exists check, then allow this function or class to be defined. | ||
| if ( T_STRING === $token[0] && ( 'function_exists' === $token[1] || 'class_exists' === $token[1] ) ) { | ||
| $type = 'function_exists' === $token[1] ? T_FUNCTION : T_CLASS; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy lift
Add client coverage for the refreshed Run Once nonce.
The PHP test covers the Heartbeat producer only. Add client tests for
src/js/utils/restAPI.ts:78-85andsrc/js/components/ManageMenu/SnippetsTable/TableColumns.tsx:41-44. Without these checks, a client regression can leave open pages using an expired nonce.🤖 Prompt for AI Agents