-
Notifications
You must be signed in to change notification settings - Fork 79
Add _meta Field #129
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
base: main
Are you sure you want to change the base?
Add _meta Field #129
Conversation
| public function __construct( | ||
| public ?string $name = null, | ||
| public ?string $description = null, | ||
| public ?array $_meta = null |
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.
You could name the PHP variables $meta instead to stay consistent with how meta is handled in the Request class, and then when serializing to and from array, use the _meta key where neccesary
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.
Yes I could have but :
- it seems to me it was inconsistent before cause ListRootsResult and InitializeResult are using _meta
- I thought it was easier to be consistent with the naming of the shema
- And now there is a lot more _meta than meta ;-)
I hope you don't mind
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.
I agree with @CodeWithKyrian here, so let's fix that with #134
src/Schema/ResourceTemplate.php
Outdated
| mimeType: $data['mimeType'] ?? null, | ||
| annotations: isset($data['annotations']) ? Annotations::fromArray($data['annotations']) : null | ||
| annotations: isset($data['annotations']) ? Annotations::fromArray($data['annotations']) : null, | ||
| _meta: isset($data['_meta']) ? (int) $data['_meta'] : null |
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.
Same int cast issue here
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.
Corrected also
|
And this time I did run all the tests also so it should pass the check. |
Add _meta field to schema
Motivation and Context
Field meta from the spec is missing
https://modelcontextprotocol.io/specification/2025-06-18/basic#meta
How Has This Been Tested?
Tested with MCP Inspector
Breaking Changes
No
Types of changes
Checklist