Skip to content

[#152] Remove pluginfile tokens #153

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.

Sign up for GitHub

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

Open
owenherbert-catalyst wants to merge 1 commit into
base: MOODLE_404_STABLE
Choose a base branch
from
Open

[#152] Remove pluginfile tokens #153

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions classes/document.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,9 @@ protected function get_text_format() {
* @return string HTML text to be renderer
*/
protected function format_text($text) {
// Remove @@PLUGINFILE@@ tokens and associated file paths from search snippets.

@dmitriim dmitriim Aug 3, 2026

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.

@owenherbert-catalyst , do I understand this correctly? We remove the token because there is no way to reconstruct the full plugin file URL?

I'm also wondering what the real-world use case is for reproducing this issue. The testing instructions seem to rely on a fairly artificial scenario.

@owenherbert-catalyst owenherbert-catalyst Aug 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Hey @dmitriim , I created a core tracker for this a while ago. This can be seen here. This issue can be reproduced in all search plugins not just Elastic search.

I believe the more correct approach is to address it in core.

@dmitriim dmitriim Aug 3, 2026 •
edited
Loading

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.

Yes. That was my thought. I think we should close this PR and instead fix it in core.

$text = preg_replace('/@@PLUGINFILE@@[^\s"\'<>\]]*/', '', $text);

// Since we allow output for highlighting, we need to encode html entities.
// This ensures plaintext html chars don't become valid html.
$out = s($text);
Expand Down
36 changes: 36 additions & 0 deletions tests/document_test.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,42 @@ public function test_highlight_text_multiple(): void {
$this->assertEquals($expected, $proxy);
}

/**
* Test that @@PLUGINFILE@@ tokens and associated file paths are stripped from text.
*/
public function test_pluginfile_token_stripped(): void {
$text = 'some content with a file @@PLUGINFILE@@/path/to/file.jpg and some more text';
$expected = 'some content with a file and some more text';

$builder = $this->getMockBuilder('\search_elastic\document');
$builder->disableOriginalConstructor();
$stub = $builder->getMock();

$method = new \ReflectionMethod('\search_elastic\document', 'format_text');
$method->setAccessible(true);
$proxy = $method->invoke($stub, $text);

$this->assertEquals($expected, $proxy);
}

/**
* Test that @@PLUGINFILE@@ with highlighting markers is handled correctly.
*/
public function test_pluginfile_with_highlighting(): void {
$text = '@@PLUGINFILE@@/path/to/file.jpg @@HI_S@@search term@@HI_E@@ description';
$expected = ' <span class="highlight">search term</span> description';

$builder = $this->getMockBuilder('\search_elastic\document');
$builder->disableOriginalConstructor();
$stub = $builder->getMock();

$method = new \ReflectionMethod('\search_elastic\document', 'format_text');
$method->setAccessible(true);
$proxy = $method->invoke($stub, $text);

$this->assertEquals($expected, $proxy);
}

/**
* Test getting enrichment processors.
*/
Expand Down