-
-
Notifications
You must be signed in to change notification settings - Fork 70
Improve escaping text in handlebars, with a focus on extensibility. #306
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: master
Are you sure you want to change the base?
Improve escaping text in handlebars, with a focus on extensibility. #306
Conversation
| { | ||
| Headers = Array.Empty<GenericFormOptionValue>(); | ||
| Fields = Array.Empty<GenericFormOptionValue>(); | ||
| // The MediaType is left as JSON because that is what is assumed to be used in GenericFormClient.cs |
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.
Json is not assuimed in GenericFormClient, the content type should be application/x-www-form-urlencoded
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.
It may not be intended to be, but the body is currently treated solely as JSON per current master: https://github.com/jellyfin/jellyfin-plugin-webhook/blob/c507d/Jellyfin.Plugin.Webhook/Destinations/GenericForm/GenericFormClient.cs#L52-L58
Once in the processing itself the Generic Client does use FormURLContent (line 77 of same file, unchanged in PR)
| /// <summary> | ||
| /// Gets or sets the Media-Content type of the webhook body. Should not be set by User except for Generic Client. | ||
| /// </summary> | ||
| public WebhookMediaContentType MediaContentType { get; set; } = WebhookMediaContentType.Json; |
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.
This should probably possible to set in the webui
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 do have it available to set in the Generic Client via a header, though at the moment it only supports json, xml, and generic/plaintext. For the individual pre-built clients though I think it makes sense to keep it hidden, or at least on a per-client basis. Discord webhooks for example only allow for JSON, so it makes no sense to expose an option that invariably breaks functionality if changed.
Co-authored-by: Cody Robibero <[email protected]>
These changes are based on those provided by @stoggi in #221 but with additional changes and movements based on suggestions from @crobibero.
Changes:
WebhookMediaContentType.csan Enum that lists out the most common text encodings for webhooks (Plaintext,Json, andXml)BaseOption.csto escape Json properly. Xml and Plaintext are left to use the existing escape function as they had been.BaseOption.csDataObjectHelpers.csThis PR will:
This PR may: