Skip to content

Fix parameter-level schema output by returning raw definition #56

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Aug 5, 2025

Conversation

judus
Copy link
Contributor

@judus judus commented Jul 30, 2025

This PR fixes issue #55, where parameter-level #[Schema(...)] attributes incorrectly wrapped the definition inside a 'definition' key

@CodeWithKyrian
Copy link
Contributor

Hey man, thanks for the PR.

You're absolutely right that there's a problem with how the definition key is being handled at the parameter level. The current implementation is wrapping it in a definition key instead of using it as a complete schema override, which breaks the intended behavior.

Here's what's happening:

Method level (working correctly):

  • When definition is set, it takes full precedence and bypasses all auto-inference
  • The schema generator returns the definition directly without any wrapping

Parameter level (the bug):

  • The extractParameterLevelSchema() method calls toArray() on the Schema attribute
  • This wraps the definition in a definition key instead of returning it directly
  • So instead of getting the complete schema, we get {definition: {...}} which then gets merged with inferred schemas

Your fix addresses the parameter-level issue, but it creates a new problem: now method-level definition gets expanded and merged with inferred schemas, which breaks the "complete override" promise we make in the docs.

The proper fix is to handle this in buildParameterSchema() instead. When we get the parameter-level schema from paramInfo['parameter_schema'], we should check if it has a definition key. If it does, we use that definition directly (discarding all inferred schemas). If not, we continue with the normal merging where parameter-level takes highest precedence.

This way:

  • Method-level definition continues to work as a complete override
  • Parameter-level definition also works as a complete override
  • Parameter-level non-definition schemas still merge properly with inferred schemas
  • The precedence system stays intact

Would you be open to adjusting your PR to implement this approach instead? The key insight is that definition should always mean "use this schema exactly as-is, no merging" regardless of whether it's at method or parameter level.

Thanks again for flagging this.

@judus judus force-pushed the fix/schema-definition-output branch from be3010f to 5b1087a Compare August 5, 2025 09:55
@judus
Copy link
Contributor Author

judus commented Aug 5, 2025

Hi, thanks for the thorough review.

You're absolutely right, and I apologize for not digging deep enough. Thank you for clarifying. I’ve updated the implementation to follow your suggested approach:

  • The toArray() method remains unchanged to preserve method-level behavior.
  • Parameter-level definition is now handled in buildParameterSchema().
  • The test and fixture were kept, but split into a separate commit.

@CodeWithKyrian CodeWithKyrian merged commit 4b34a02 into php-mcp:main Aug 5, 2025
4 checks passed
@judus judus deleted the fix/schema-definition-output branch August 5, 2025 13:15
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.

Bug: Parameter‑level #[Schema] wraps output in extra definition key
2 participants