-
-
Notifications
You must be signed in to change notification settings - Fork 834
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
Reading $post->content in saving listener throws error if content is empty #3983
Labels
Comments
10 tasks
@DavideIadeluca I received an email notification for a reply I suspect you now deleted. I assume you found a way to reproduce? #4059 looks good |
@clarkwinkelmann Yes, I was able to reproduce it after all. Shouldn't have deleted the comment |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Current Behavior
Trying to read
$post->content
on aCommentPost
insideFlarum\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.
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->content
should returnnull
or an empty stringScreenshots
No response
Environment
Output of
php flarum info
Possible Solution
Just like
CommentPost::setContentAttribute()
skips the formatter and sets the value tonull
directlyframework/framework/core/src/Post/CommentPost.php
Lines 113 to 115 in 270188b
CommentPost::getContentAttribute()
should also skip the formatter if the value isnull
and return it directlyframework/framework/core/src/Post/CommentPost.php
Lines 103 to 105 in 270188b
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.
framework/framework/core/src/Formatter/Formatter.php
Line 136 in 2a693db
framework/extensions/mentions/src/Formatter/UnparsePostMentions.php
Line 34 in 2a693db
In the 2.x branch, a
string $xml
has been added, so it would error right here when called withnull
, even if there are no unparsing callbacks registered.framework/framework/core/src/Formatter/Formatter.php
Line 97 in 270188b
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.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 FriendsOfFlarum/filter#52
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 post0
as the body, which is definitely not desirable behaviour.The text was updated successfully, but these errors were encountered: