Skip to content
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

Standardizing a way for custom post types to output content as text or HTML #218

Open
clarkwinkelmann opened this issue Mar 30, 2020 · 7 comments

Comments

@clarkwinkelmann
Copy link
Member

I create this issue to discuss possible ways to implement custom post types that contain text.

Where is the data used ?

  • $post->content is used in emails
  • The content might be used in web notifications but I don't think that's the case currently
  • $post->formatContent() is used by the SEO extension
  • Other extensions might use the data for custom notifications or indexing

Which posts are accessed ?

  • Most impacted emails are triggered via the Posted event, which is only dispatched for CommentPost by Flarum, but an extension might dispatch it with another post type. The type hinting on the event allows any post type
  • The SEO extension and FoF Follow tags access $discussion->firstPost. They both currently don't check that the post is a CommentPost before reading the content, but checking for that class explicitly would break the ability to do what this issue is about

Concerns

  • Extensions need a way to know whether a $post object can be rendered as text or HTML. Related Can't handle non-comments as first post FriendsOfFlarum/follow-tags#7
  • A post might be renderable as text or HTML, but it might not use the TextFormatter for that
  • Custom post types must be able to declare whether they can be rendered as text or HTML
  • At the moment, because there's almost no checks for CommentPost, Flarum and extensions will happily call ->formatContent() on any post. This has the benefit of allowing custom post types to use it, but also throws errors if another post type ends up there
  • The use of ->content for the plain text version conflicts with the access to the ->content column of the database. It's not an issue for comment posts because there's nothing else to access, but because ->content is used for the JSON data of custom post types, re-declaring getContentAttribute() prevents sending the full payload via the serializer
  • Using ->content for plain text and still being able to access JSON would be possible, but I think re-naming the accessor to something that says "text" or "plain" will be much clearer

Use cases

  • My premium Wordpress extension registers a custom post type that contains an excerpt of the linked Wordpress post. That post stores a title, html body and link. This content can be dynamically rendered as a single HTML payload for notifications and SEO. It's expected that other extensions could access this content and treat it like a comment post
  • Imagined example: an automated script pushes content from a feed into a discussion. Each post contains a source url and content. You might want to receive posting notifications from the Subscription extension in that thread

Proposals

Easy but not satisfactory solution

We could add explicit checks for CommentPost everywhere before passing it to an email template or reading the content. This would mean every third-party extension that has such a custom post type would need to define their own logic for everything, including checking the other extension (like Subscription) is enabled and compose their own email template.

Pros: very little change to Flarum, we just prevent things from breaking and let extension developers manage everything.

Cons: compatibility between third-party extensions is complicated or impossible. An extension cannot simply register a new post type and expect that another notification extensions will pick it up.

Concrete solution

Add a new interface for posts. For example:

interface Renderable {
    public contentPlain(ServerRequestInterface $request = null) {}
    public contentHtml(ServerRequestInterface $request = null) {}
}

And update core extensions that handle email to check for that interface before trying to read the content.

One question regards the "plain text" version. If we used HTML in the emails we could completely skip that from the interface and only have an HTML output.

Another question is the naming. I don't really like the current names. I would like something that clearly says "whatever the post payload is it can be rendered as a single text". The intent of that interface would be to be used by default for everything notification or indexing related unless additional logic has been written for a specific post type.

This solution only covers the PHP side. Rendering the content in the javascript frontend would still require custom code for those special post types. But I don't think there are any major issues there, the frontend is sufficiently extensible for the developer to choose how such a post will be rendered.

@stale

This comment has been minimized.

@stale stale bot added the stale label Jun 28, 2020
@dsevillamartin

This comment has been minimized.

@stale stale bot removed the stale label Jun 28, 2020
@stale

This comment has been minimized.

@stale stale bot added the stale label Sep 26, 2020
@clarkwinkelmann

This comment has been minimized.

@stale

This comment has been minimized.

@stale stale bot closed this as completed Oct 28, 2020
@clarkwinkelmann
Copy link
Member Author

Uh. The bot closed even though labels were assigned?

@stale stale bot removed the stale label Oct 29, 2020
@dsevillamartin
Copy link
Member

It will not close only with specific tags.

@askvortsov1 askvortsov1 transferred this issue from flarum/framework Mar 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants