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

Port from City Tech OpenLab: Rich text and media embedding for comments #393

Open
bree-z opened this issue May 12, 2022 · 8 comments
Open
Assignees
Labels
OpenLab Issues specifically related to the OpenLab package

Comments

@bree-z
Copy link

bree-z commented May 12, 2022

Can we add these plugins to CBOX OpenLab?

Thanks!

@bree-z bree-z added the OpenLab Issues specifically related to the OpenLab package label May 12, 2022
@bree-z bree-z added this to the 1.4.0 milestone May 12, 2022
@boonebgorges
Copy link
Member

Possibly. We will need to make decisions about how they get included, and specifically whether they're required and/or network-activated by default. Let's see how our summer resources shake out before moving forward.

@r-a-y
Copy link
Member

r-a-y commented May 12, 2022

Not sure how CBOX-OL is handling media embedding for comments, but I wrote an oEmbed plugin for comments that's quite simple: https://github.com/r-a-y/oembed-for-comments

@boonebgorges
Copy link
Member

Thanks for chiming in, @r-a-y ! Your plugin was actually a starting point for what we ended up building for the OpenLab. It has some additional changes to account for guest-comments vs member-comments, and also compatibility layers for wp-grade-comments and the rich text comment tool we're working on. See https://github.com/openlab-at-city-tech/openlab/blob/1.7.x/wp-content/plugins/openlab-oembed-comments/openlab-oembed-comments.php

A question for you - You say https://github.com/r-a-y/oembed-for-comments/blob/master/oembed-for-comments.php#L85= that the shortcode() method needs to be overridden to account for comments rather than posts. Do you recall the details of this?

@r-a-y
Copy link
Member

r-a-y commented May 24, 2022

A question for you - You say https://github.com/r-a-y/oembed-for-comments/blob/master/oembed-for-comments.php#L85= that the shortcode() method needs to be overridden to account for comments rather than posts. Do you recall the details of this?

Yes, my oembed-for-comments caches oEmbed results into comment meta, so it needs to reference the comment ID and save to comment meta rather than the post ID and post meta. See https://github.com/WordPress/WordPress/blob/5214e469df26317c60b23d0b4719b787474d3d32/wp-includes/class-wp-embed.php#L195.

Some changes that WordPress has made to the WP_Embed class since I've written my plugin is introduce oEmbed cache expiry: https://github.com/WordPress/WordPress/blob/5214e469df26317c60b23d0b4719b787474d3d32/wp-includes/class-wp-embed.php#L246. Right now, my plugin caches once and doesn't expire. So that could be added as an enhancement.

It has some additional changes to account for guest-comments vs member-comments

I see that if a guest is posting a comment, oEmbed is disabled: https://github.com/openlab-at-city-tech/openlab/blob/412253756ae599c022e81416209dc24ff7daacc6/wp-content/plugins/openlab-oembed-comments/openlab-oembed-comments.php#L98-L100. Is that the main purpose for guest comments?

One other thing I see is allowing the <iframe> element: https://github.com/openlab-at-city-tech/openlab/blob/412253756ae599c022e81416209dc24ff7daacc6/wp-content/plugins/openlab-oembed-comments/openlab-oembed-comments.php#L113. Is this necessary for rich-text or certain embeds? If the embeds are targeted to a specific website, maybe an embed provider can be written for it instead?

@boonebgorges
Copy link
Member

Yes, my oembed-for-comments caches oEmbed results into comment meta, so it needs to reference the comment ID and save to comment meta rather than the post ID and post meta.

Ah yes. I wonder whether this is strictly necessary, though. During normal comment rendering (ie in the comment section on the front end) the get_post() will always return the comment's post. You could then have cases where multiple comments belonging to the same post share the same cache for the same URL asset, but this feels like a pretty minimal risk. (In fact, you might call it overkill that WP caches a given embed separately on a per-post basis.)

Is that the main purpose for guest comments?

Yep, that's it. This was what the OpenLab folks wanted.

Is this necessary for rich-text or certain embeds? If the embeds are targeted to a specific website, maybe an embed provider can be written for it instead?

I'll have to look back to be certain, but I think it ensures that we support arbitrary embeds, such as those that are enabled by third-party plugins. That is, not oEmbed but regular embeds. So I'm not sure that one-off handlers would cover it.

@r-a-y
Copy link
Member

r-a-y commented May 25, 2022

During normal comment rendering (ie in the comment section on the front end) the get_post() will always return the comment's post.

It's more about segregating the respective data. Might be an edge case, but imagine a scenario with a rather, large comment thread with a ton of oEmbed links. Right now, all that data would be stored in postmeta and would be pulled in whenever a postmeta cache call is used.

One other thing I see is allowing the <iframe> element: https://github.com/openlab-at-city-tech/openlab/blob/412253756ae599c022e81416209dc24ff7daacc6/wp-content/plugins/openlab-oembed-comments/openlab-oembed-comments.php#L113.

Upon looking at the the 'olgc_private_comment_text' filter, correct me if I'm wrong, but it doesn't look as though this filter exists on the OpenLab anymore. So maybe the <iframe> code is left over from a previous implementation of WP Grade Comments?

@boonebgorges
Copy link
Member

It's more about segregating the respective data. Might be an edge case, but imagine a scenario with a rather, large comment thread with a ton of oEmbed links. Right now, all that data would be stored in postmeta and would be pulled in whenever a postmeta cache call is used.

This is helpful context.

Upon looking at the the 'olgc_private_comment_text' filter, correct me if I'm wrong, but it doesn't look as though this filter exists on the OpenLab anymore. So maybe the <iframe> code is left over from a previous implementation of WP Grade Comments?

Actually, it's a new filter that hasn't yet made it into the production version. And now that I look at it, it might be linked at least in part to rich-text comment work that we're doing.

@boonebgorges boonebgorges modified the milestones: 1.4.0, 1.5.0 Jan 9, 2023
@bree-z bree-z modified the milestones: 1.5.0, Future Release Jun 6, 2023
@bree-z
Copy link
Author

bree-z commented Jun 6, 2023

Hi Boone,

I'm moving this to the future release milestone, to revisit after we implement the changes in 3180.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OpenLab Issues specifically related to the OpenLab package
Projects
None yet
Development

No branches or pull requests

3 participants