Trying to read $post->content on a CommentPost inside Flarum\Post\Event\Saving will throw a 500 error if the user submitted an empty content value.
The problem would also happen when trying to read that attribute anytime if an extension allowed a comment post to be saved with an empty post, which Flarum doesn't allow by default.
I did my tests on Flarum 1.8 but I am certain the problem is still present in the 2.x branch because the code hasn't changed.
Enable any extension that registers a formatter unparsing callback, like Mentions.
Add local extender, or use in an extension:
<?php
use Flarum\Extend;
return [
// Register extenders here to customize your forum!
(new Extend\Event())
->listen(\Flarum\Post\Event\Saving::class, function ($event) {
// Just need to call the magic getter method, no need to do anything with the result
$event->post->content;
}),
];
Try creating discussion or reply without any body. See error.
Expected Behavior
$post->content should return null or an empty string
Screenshots
No response
Environment
Flarum version: 1.8.5
Webserver: apache
Hosting environment: local
PHP version: 8.1.28
Output of php flarum info
Output of "php flarum info", run this in terminal in your Flarum directory.
Possible Solution
Just like CommentPost::setContentAttribute() skips the formatter and sets the value to null directly
The error trace will be different between Flarum 1.8 and 2.x, because in 1.8.5, Formatter::unparse() has no type check for $xml.
Because of that, the type error is only thrown when encountering the first unparse callback that type-hints its parameters according to the extender documentation, like Mentions does.
In the 2.x branch, a string $xml has been added, so it would error right here when called with null, even if there are no unparsing callbacks registered.
I'd like to take this opportunity to suggest always parsing content, even empty strings. I don't have any concrete use case, but I feel like it would be more logical to always treat a comment post content as string. It would ensure any logic registered through TextFormatter runs on all posts by default. Extensions that don't want this behavior would have an easier time saving null in the database in the event listener rather than having to call the formatter themselves on the possibly empty content. I think this would require changes to the validator because since it looks at the XML which would now always have a value.
In any case, the getter needs to not error on null values because it's a valid value that can be set in the database.
EDIT: adding this as a footnote here as I don't have time to create a separate issue, but the $value ? /*...*/ : null condition also means you can't post 0 as the body, which is definitely not desirable behaviour.
Current Behavior
Trying to read
$post->contenton aCommentPostinsideFlarum\Post\Event\Savingwill throw a 500 error if the user submitted an empty content value.The problem would also happen when trying to read that attribute anytime if an extension allowed a comment post to be saved with an empty post, which Flarum doesn't allow by default.
I did my tests on Flarum 1.8 but I am certain the problem is still present in the 2.x branch because the code hasn't changed.
Steps to Reproduce
Enable any extension that registers a formatter unparsing callback, like Mentions.
Add local extender, or use in an extension:
Try creating discussion or reply without any body. See error.
Expected Behavior
$post->contentshould returnnullor an empty stringScreenshots
No response
Environment
Output of
php flarum infoPossible Solution
Just like
CommentPost::setContentAttribute()skips the formatter and sets the value tonulldirectlyhttps://github.com/flarum/framework/blob/270188b5b01c55c30b481eeb565ff5be05143572/framework/core/src/Post/CommentPost.php#L113-L115
CommentPost::getContentAttribute()should also skip the formatter if the value isnulland return it directlyhttps://github.com/flarum/framework/blob/270188b5b01c55c30b481eeb565ff5be05143572/framework/core/src/Post/CommentPost.php#L103-L105
The error trace will be different between Flarum 1.8 and 2.x, because in 1.8.5,
Formatter::unparse()has no type check for$xml.Because of that, the type error is only thrown when encountering the first unparse callback that type-hints its parameters according to the extender documentation, like Mentions does.
https://github.com/flarum/framework/blob/2a693db1b65cf7b0f7e76cbaa695a5e65bd947f6/framework/core/src/Formatter/Formatter.php#L136
https://github.com/flarum/framework/blob/2a693db1b65cf7b0f7e76cbaa695a5e65bd947f6/extensions/mentions/src/Formatter/UnparsePostMentions.php#L34
In the 2.x branch, a
string $xmlhas been added, so it would error right here when called withnull, even if there are no unparsing callbacks registered.https://github.com/flarum/framework/blob/270188b5b01c55c30b481eeb565ff5be05143572/framework/core/src/Formatter/Formatter.php#L97
I'd like to take this opportunity to suggest always parsing content, even empty strings. I don't have any concrete use case, but I feel like it would be more logical to always treat a comment post content as string. It would ensure any logic registered through TextFormatter runs on all posts by default. Extensions that don't want this behavior would have an easier time saving
nullin the database in the event listener rather than having to call the formatter themselves on the possibly empty content. I think this would require changes to the validator because since it looks at the XML which would now always have a value.In any case, the getter needs to not error on
nullvalues because it's a valid value that can be set in the database.Additional Context
Reported and discussed at https://discuss.flarum.org/d/34454-every-day-there-are-some-discussions-only-have-title-no-content
FoF Filter issue about its impact https://github.com/FriendsOfFlarum/filter/issues/52
EDIT: adding this as a footnote here as I don't have time to create a separate issue, but the
$value ? /*...*/ : nullcondition also means you can't post0as the body, which is definitely not desirable behaviour.