Skip to content

Add PHP DTO converter with round-trip support#2

Merged
voku merged 3 commits into
mainfrom
copilot/add-php-dtos-generation-support
Apr 18, 2026
Merged

Add PHP DTO converter with round-trip support#2
voku merged 3 commits into
mainfrom
copilot/add-php-dtos-generation-support

Conversation

Copy link
Copy Markdown

Copilot AI commented Apr 18, 2026

  • Inspect packages/typediagram/src/converters/php.ts and current PHP converter tests
  • Verify current typediagram-core build, typecheck, and test baseline
  • Fix valid PHP converter issues from review feedback: top-level splitting around strings/comments, namespaced type parsing, double-quoted kind literals, Unit field mapping, and array<T> docblock interpretation
  • Add/adjust PHP converter tests to prove each accepted fix
  • Re-run npm run -w typediagram-core build, typecheck, and test
  • Run final review/security validation (Code Review clean aside from non-blocking readability nits; CodeQL timed out on the last pass after earlier security issues were removed)

Copilot AI and others added 2 commits April 18, 2026 03:46
@voku voku marked this pull request as ready for review April 18, 2026 03:55
@voku
Copy link
Copy Markdown
Owner

voku commented Apr 18, 2026

/gemini review

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces PHP support to the typeDiagram ecosystem, including a new bidirectional converter for PHP DTOs, CLI integration, web-based syntax highlighting, and updated documentation. The PHP converter implementation requires several improvements to its parsing logic, specifically regarding the handling of string literals and comments in splitTopLevel, support for namespaced types in parameter parsing, and the inclusion of double-quoted strings for union variant discriminators. Additionally, the mapping of the Unit type to void for parameters needs correction to comply with PHP standards, and the interpretation of single-argument array docblocks should be updated to map to List instead of a Map.

Comment on lines +91 to +108
const splitTopLevel = (source: string, openChar: string, closeChar: string): string[] => {
const parts: string[] = [];
let depth = 0;
let start = 0;
for (let i = 0; i < source.length; i++) {
const char = source.charAt(i);
depth = adjustDepth(depth, char, openChar, closeChar);
if (char === "," && depth === 0) {
const part = source.slice(start, i).trim();
if (part.length > 0) {
parts.push(part);
}
start = i + 1;
}
}
const last = source.slice(start).trim();
return last.length > 0 ? [...parts, last] : parts;
};
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The splitTopLevel function does not account for string literals or comments. If a parameter list or generic argument list contains a comma inside a string (e.g., public string $msg = "Hello, world"), this function will incorrectly split the string, leading to parsing failures for subsequent fields. A more robust implementation should track whether the cursor is inside a string and skip delimiters accordingly.

return { nativeType: "array", docType: mapTdToPhpDocType(type), hasDefaultNull: false };
}
if (type.name === "Unit") {
return { nativeType: "mixed", docType: "void", hasDefaultNull: false };
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Mapping Unit to void for parameters or properties is invalid in PHP. In PHP, void is strictly a return type hint. Using it in a docblock as @param void $field will cause errors in static analysis tools like PHPStan or Psalm. Consider mapping Unit to null or mixed for DTO fields.

const docTypes = parseParamDocsFromDocblock(constructorDoc);
return splitParams(source)
.map((part) => {
const match = /public\s+([?A-Za-z_][A-Za-z0-9_]*)\s+\$(\w+)(?:\s*=\s*null)?/.exec(part.replace(/\s+/g, " "));
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The regex for parsing parameters does not support namespaced types (e.g., \App\DTO\User). PHP types frequently include namespaces. The character class [?A-Za-z_] should be expanded to include the backslash \ character. Also, note that replace(/\s+/g, " ") on the entire parameter string can corrupt string literals containing multiple spaces.

Comment on lines +401 to +404
return keyType === undefined || valueType === undefined
? "Map<String, String>"
: `Map<${mapPhpDocTypeToTd(keyType)}, ${mapPhpDocTypeToTd(valueType)}>`;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

In PHPStan/Psalm, array<T> is the standard way to denote a list of type T. The current implementation incorrectly defaults to Map<String, String> when only one generic argument is provided. It should map to List<T> in this case to preserve the intended semantics.

const toTypeRef = (param: ParsedParam) =>
parseTypeRef(param.docType === null ? mapPhpNativeTypeToTd(param.nativeType) : mapPhpDocTypeToTd(param.docType));

const parseKindLiteral = (body: string) => /@var\s+'([^']+)'[\s\S]*?public string \$kind;/.exec(body)?.[1] ?? null;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The regex for parsing the $kind literal is restricted to single quotes. PHP allows both single and double quotes for strings. This will fail to parse union variants if double quotes are used in the docblock (e.g., /** @var "Circle" */).

Agent-Logs-Url: https://github.com/voku/typeDiagram/sessions/e1ea5ebf-8997-4e24-b55d-3ff1514ed8c5

Co-authored-by: voku <264695+voku@users.noreply.github.com>
Copilot AI requested a review from voku April 18, 2026 04:17
Copilot finished work on behalf of voku April 18, 2026 04:17
@voku
Copy link
Copy Markdown
Owner

voku commented Apr 18, 2026

/gemini review

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request adds PHP as a supported language across the typeDiagram platform, implementing a new bidirectional converter, CLI arguments, and web interface components. The implementation includes support for records, unions, and aliases, along with comprehensive integration tests and updated documentation. The review feedback highlights several areas for improving the PHP parser's robustness, including better handling of nested delimiters in constructor parameters, support for namespaced types in declaration regexes, and more reliable extraction of kind literals from docblocks.

Comment on lines +82 to +90
const adjustDepth = (depth: number, char: string, openChar: string, closeChar: string) => {
if (char === openChar) {
return depth + 1;
}
if (char === closeChar) {
return depth - 1;
}
return depth;
};
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The adjustDepth function currently only tracks the specific openChar and closeChar passed to it. This causes issues when splitting constructor parameters if a default value contains other delimiters, such as short array syntax []. For example, public array $v = [1, 2] would be incorrectly split at the comma inside the array because the depth counter wouldn't increment for [. It is better to track all common PHP delimiters to correctly identify the top-level scope for splitting.

const adjustDepth = (depth: number, char: string, openChar: string, closeChar: string) => {
  if ([openChar, "(", "[", "{", "<"].includes(char)) return depth + 1;
  if ([closeChar, ")", "]", "}", ">"].includes(char)) return depth - 1;
  return depth;
};


const parseDeclarations = (source: string): ParsedDecl[] => {
const declarations: ParsedDecl[] = [];
const declarationRe = /interface\s+(\w+)\s*\{|final readonly class\s+(\w+)(?:\s+implements\s+(\w+))?\s*\{/g;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The regex for parsing declarations uses \w+ for names, which prevents it from matching namespaced interfaces in the implements clause (e.g., implements \App\Interfaces\Identifiable). Since DTOs are frequently namespaced in PHP, the regex should be updated to support backslashes in identifiers.

Suggested change
const declarationRe = /interface\s+(\w+)\s*\{|final readonly class\s+(\w+)(?:\s+implements\s+(\w+))?\s*\{/g;
const declarationRe = /interface\s+([\\a-zA-Z0-9_]+)\s*\{|final readonly class\s+([\\a-zA-Z0-9_]+)(?:\s+implements\s+([\\a-zA-Z0-9_]+))?\s*\{/g;

Comment on lines +514 to +516
const literalStart = varIndex + "@var ".length;
const quote = body.charAt(literalStart);
return quote === "'" || quote === '"' ? readQuotedLiteral(body, literalStart + 1, quote) : null;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The logic for extracting the @var literal is fragile as it assumes the quoted value starts immediately after the @var string. This will fail if there is a newline or extra whitespace (such as the * prefix in multi-line docblocks). Using a regular expression is more robust.

Suggested change
const literalStart = varIndex + "@var ".length;
const quote = body.charAt(literalStart);
return quote === "'" || quote === '"' ? readQuotedLiteral(body, literalStart + 1, quote) : null;
const match = /@var\s+(['"])(.*?)\1/.exec(body.slice(varIndex));
return match ? match[2] : null;

Copilot finished work on behalf of voku April 18, 2026 04:40
@voku voku merged commit 4f877d3 into main Apr 18, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants