Skip to content

KB : video embed support via slash command#24268

Open
f2cmb wants to merge 13 commits into
glpi-project:mainfrom
f2cmb:kb/editor/videoEmbed
Open

KB : video embed support via slash command#24268
f2cmb wants to merge 13 commits into
glpi-project:mainfrom
f2cmb:kb/editor/videoEmbed

Conversation

@f2cmb

@f2cmb f2cmb commented May 20, 2026

Copy link
Copy Markdown
Contributor
  • I have read the CONTRIBUTING document.
  • I have performed a self-review of my code.
  • I have added tests that prove my fix is effective or that my feature works.
  • This change requires a documentation update.

Description

  • It fixes Video embed roadmap#328
  • Here is a brief description of what this PR does : New Tiptap module (better than use the YT existing one + custom for other support).

YT/ Vimeo / Dailymotion only. Slash Command integration only.

Possible followup :

Give and admin the possibility to add custom providers.

Screenshot :

image image

@f2cmb f2cmb marked this pull request as ready for review May 22, 2026 08:01
@f2cmb f2cmb linked an issue May 22, 2026 that may be closed by this pull request

@AdrienClairembault AdrienClairembault left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to work fine, I didn't check the code in details as there is almost 2k lines.

Comment thread src/Glpi/Controller/Knowbase/KnowbaseItemController.php Outdated
Comment thread tests/e2e/pages/KnowbaseItemPage.ts Outdated
Comment thread tests/e2e/pages/KnowbaseItemPage.ts Outdated
Comment thread tests/e2e/pages/KnowbaseItemPage.ts Outdated
@f2cmb f2cmb requested a review from AdrienClairembault May 28, 2026 12:12
@f2cmb

f2cmb commented Jun 8, 2026

Copy link
Copy Markdown
Contributor Author

Asked for review @cedric-anne today

@cedric-anne cedric-anne added this to the 12.0.0 milestone Jun 9, 2026

@cedric-anne cedric-anne left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This represent a huge amount of code to just support youtube/vimeo/dailymotion providers.

I cannot tell how many people will want to use this feature, and I cannot tell either how many people will want to add their own self-hosted videos using just a https://example.org/myvideo.mp4 URL, but, IMHO, handling only video files URLs and render them in a <video> tag would probably cover more use cases.

Ping @orthagh

Comment thread src/Glpi/RichText/RichText.php Outdated
Comment thread src/Glpi/RichText/RichText.php Outdated
Comment thread src/Glpi/RichText/RichText.php
Comment thread src/Glpi/RichText/VideoEmbedRenderer.php
'<div class="video-embed-wrapper">'
. '<iframe src="%s" title="%s" loading="lazy" allowfullscreen'
. ' referrerpolicy="strict-origin-when-cross-origin"'
. ' sandbox="allow-scripts allow-same-origin allow-presentation allow-popups"'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is allow-same-origin necessary? As far as I understand, it will allow the iframe to access the GLPI page DOM.

Is allow-popups necessary?

Suggested change
. ' sandbox="allow-scripts allow-same-origin allow-presentation allow-popups"'
. ' sandbox="allow-scripts allow-presentation"'

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is allow-same-origin necessary?

Yes : I tested removing it and the players won't load: without it the iframe gets an opaque origin and can't read its own cookies/localStorage.

As far as I understand, it will allow the iframe to access the GLPI page DOM.

It won't : cross-origin DOM access is blocked by the Same-Origin Policy regardless of the sandbox. The allow-scripts + allow-same-origin escape only applies when the frame is same-origin with the page, and src is always a hard-coded provider host (youtube-nocookie.com / player.vimeo.com / dailymotion.com).

Is allow-popups necessary?

No, removed it (only used by the players' "watch on …" buttons). Also dropped referrerpolicy (already the browser default). Final set: allow-scripts allow-same-origin allow-presentation.

Comment thread src/Glpi/RichText/VideoEmbedRenderer.php Outdated
Comment thread tests/functional/Glpi/RichText/RichTextTest.php Outdated
Comment thread tests/functional/Glpi/RichText/VideoEmbedRendererTest.php Outdated
Comment thread tests/functional/Glpi/RichText/VideoEmbedRendererTest.php Outdated
Comment thread tests/functional/Glpi/RichText/VideoEmbedRendererTest.php
Comment thread tests/functional/Glpi/RichText/VideoEmbedRendererTest.php
Comment thread tests/functional/Glpi/RichText/VideoEmbedRendererTest.php
@f2cmb

f2cmb commented Jun 11, 2026

Copy link
Copy Markdown
Contributor Author

@cedric-anne i processed your concerns (partially for allow- comment, see explanations above.

Waiting for @orthagh decision : drop external provider, add

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Video embed

3 participants