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

How to handle embeds? #9582

Closed
hypeJunction opened this Issue Mar 28, 2016 · 20 comments

Comments

Projects
None yet
2 participants
@hypeJunction
Contributor

hypeJunction commented Mar 28, 2016

#9572 breaks our embed logic, mainly because our embed logic is flawed in that it relies on URLs to never change.

Strawman 1. Manually set icon/download URLs to use old handlers until we figure out a solution
Strawman 2. We can allow non-expiring URLs in file service (either by setting very high expiration time, or no expiration time; removing the last updated check for certain URLs)
Strawman 3. Use regex callbacks to replace URLs with dynamic equivalents

  • Add serve-icon handler once #9655 is merged
@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction

hypeJunction Mar 28, 2016

Contributor

PR for Strawman 2: #9583

Contributor

hypeJunction commented Mar 28, 2016

PR for Strawman 2: #9583

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Mar 28, 2016

Member

Perpetual access to a file despite its access level has its use cases but I think is a bad idea for embeds, which embed real site entities. It's annoying when users break posts by hiding embedded images, but it's sometimes necessary. So I'm strongly in favor of 1 for now.

Member

mrclay commented Mar 28, 2016

Perpetual access to a file despite its access level has its use cases but I think is a bad idea for embeds, which embed real site entities. It's annoying when users break posts by hiding embedded images, but it's sometimes necessary. So I'm strongly in favor of 1 for now.

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Mar 28, 2016

Member

Strawman 4: new embed URL handler like the old behavior: URL just has GUID. Server verifies visibility and sends expiration of a week, with Cache-Control: private.

Member

mrclay commented Mar 28, 2016

Strawman 4: new embed URL handler like the old behavior: URL just has GUID. Server verifies visibility and sends expiration of a week, with Cache-Control: private.

@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction

hypeJunction Mar 28, 2016

Contributor

Strawman 1 will generate deprecation notices for old and new embeds.
Strawman 4 would have to go into core - we want posts to display properly even with embed plugin disabled. We can repurpose DownloadFileHandler (EmbedFileHandler, embed-file) add the $size parameter, boot core and trigger a hook, which will return ElggFile to serve.

Contributor

hypeJunction commented Mar 28, 2016

Strawman 1 will generate deprecation notices for old and new embeds.
Strawman 4 would have to go into core - we want posts to display properly even with embed plugin disabled. We can repurpose DownloadFileHandler (EmbedFileHandler, embed-file) add the $size parameter, boot core and trigger a hook, which will return ElggFile to serve.

@hypeJunction hypeJunction added this to the Elgg 2.x milestone Mar 28, 2016

@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction

hypeJunction Mar 28, 2016

Contributor

It makes me regret we abandoned ECML.

Contributor

hypeJunction commented Mar 28, 2016

It makes me regret we abandoned ECML.

@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction

hypeJunction Mar 28, 2016

Contributor

If we go down route 4, I would prefer we make it work with all entity types. We could use something like a web component, which resolves a <a href="embed/$guid?embed">$title</a> to embed markup client-side, using a hookable Ajax request.

Contributor

hypeJunction commented Mar 28, 2016

If we go down route 4, I would prefer we make it work with all entity types. We could use something like a web component, which resolves a <a href="embed/$guid?embed">$title</a> to embed markup client-side, using a hookable Ajax request.

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Mar 28, 2016

Member

Can you find our last discussion about ECML?

Member

mrclay commented Mar 28, 2016

Can you find our last discussion about ECML?

@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction
Contributor

hypeJunction commented Mar 28, 2016

@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction

hypeJunction Mar 28, 2016

Contributor

oEmbed could be an option here, but I find it weird to combine oEmbed with access controlled content. We would have to share cookies etc.

Contributor

hypeJunction commented Mar 28, 2016

oEmbed could be an option here, but I find it weird to combine oEmbed with access controlled content. We would have to share cookies etc.

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Mar 28, 2016

Member

Wouldn't oEmbed still cost a request that boots the server?

Member

mrclay commented Mar 28, 2016

Wouldn't oEmbed still cost a request that boots the server?

@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction

hypeJunction Mar 28, 2016

Contributor

It would. ECML is an ideal solution, and has proven to work with BB codes all over the internet, so I would be in favor of reviving it and making proper use of it. Or we could implement ECML 2.0 as a web component that parses a certain tag (<a> seems most suitable) and replaces markup server-side. The pitfall is that user won't see actual embedded content in WYSIWYG (unless we teach it to display it and then strip it before sending to server), e.g. add <iframe>, hide <a>, strip <iframe> when saving.

Contributor

hypeJunction commented Mar 28, 2016

It would. ECML is an ideal solution, and has proven to work with BB codes all over the internet, so I would be in favor of reviving it and making proper use of it. Or we could implement ECML 2.0 as a web component that parses a certain tag (<a> seems most suitable) and replaces markup server-side. The pitfall is that user won't see actual embedded content in WYSIWYG (unless we teach it to display it and then strip it before sending to server), e.g. add <iframe>, hide <a>, strip <iframe> when saving.

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Mar 28, 2016

Member

I think your never-expiring URL is the most sane option, but we should give content owners more control and information about the consequences of doing this. I mean, a user could always save a content-controlled image and re-upload it on the site or elsewhere.

Member

mrclay commented Mar 28, 2016

I think your never-expiring URL is the most sane option, but we should give content owners more control and information about the consequences of doing this. I mean, a user could always save a content-controlled image and re-upload it on the site or elsewhere.

@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction

hypeJunction Mar 28, 2016

Contributor

And next time we change something in URL structures, we are back at square one.

Contributor

hypeJunction commented Mar 28, 2016

And next time we change something in URL structures, we are back at square one.

@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction

hypeJunction Mar 28, 2016

Contributor

Strawman 5: Use generic embed/$file_guid/$size pattern with route:rewrite hook.

Contributor

hypeJunction commented Mar 28, 2016

Strawman 5: Use generic embed/$file_guid/$size pattern with route:rewrite hook.

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Mar 28, 2016

Member

👍 for (5)

Member

mrclay commented Mar 28, 2016

👍 for (5)

@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction

hypeJunction Mar 28, 2016

Contributor

We would have to boot the engine for 5, but we do it anyway.

Contributor

hypeJunction commented Mar 28, 2016

We would have to boot the engine for 5, but we do it anyway.

@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction

hypeJunction Mar 28, 2016

Contributor

Also how do we avoid sniffing the context in file_set_icon_url()? Should we add a hook to embed plugin to do the sniffing and rewriting thumb URLs?

Contributor

hypeJunction commented Mar 28, 2016

Also how do we avoid sniffing the context in file_set_icon_url()? Should we add a hook to embed plugin to do the sniffing and rewriting thumb URLs?

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Mar 28, 2016

Member

I'm not too familiar with this process, but sounds reasonable.

Member

mrclay commented Mar 28, 2016

I'm not too familiar with this process, but sounds reasonable.

@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction

hypeJunction Mar 28, 2016

Contributor

Embed item view calls elgg_view_entity_icon(), when user clicks on the item, JS code parses out the img URL and embeds it into the editor. We can't explicitly set entity icon URL (I think), so we have to use entity:icon:url hook.

Contributor

hypeJunction commented Mar 28, 2016

Embed item view calls elgg_view_entity_icon(), when user clicks on the item, JS code parses out the img URL and embeds it into the editor. We can't explicitly set entity icon URL (I think), so we have to use entity:icon:url hook.

@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction

hypeJunction Apr 1, 2016

Contributor

PR for Strawman 5 - #9603

Contributor

hypeJunction commented Apr 1, 2016

PR for Strawman 5 - #9603

@hypeJunction hypeJunction changed the title from How do handle embeds? to How to handle embeds? Apr 12, 2016

@hypeJunction hypeJunction modified the milestones: Icons/uploads revamp, Elgg 2.x Apr 12, 2016

mrclay added a commit to mrclay/Elgg-leaf that referenced this issue Apr 19, 2016

feature(embed): adds serve-icon page handler
Adds a handler to serve embedded icons. Adds elgg_get_embed_url() to
generate embed URLs

Fixes #9582

mrclay added a commit to mrclay/Elgg-leaf that referenced this issue Apr 20, 2016

feature(embed): adds serve-icon page handler
Adds a handler to serve embedded icons. Adds elgg_get_embed_url() to
generate embed URLs.

Adds a trait to inject the time in components for testing.

Fixes #9582

mrclay added a commit to mrclay/Elgg-leaf that referenced this issue Apr 21, 2016

feature(embed): adds serve-icon page handler
Adds a handler to serve embedded icons. Adds elgg_get_embed_url() to
generate embed URLs.

Adds a trait to inject the time in components for testing.

Fixes #9582
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment