Skip to content

Fixes issue #170 - icon images fetched via pluginfile - #173

Closed
christina-roperto wants to merge 5 commits into
jleyva:MOODLE_36_STABLEfrom
christina-roperto:pluginfile_36
Closed

Fixes issue #170 - icon images fetched via pluginfile#173
christina-roperto wants to merge 5 commits into
jleyva:MOODLE_36_STABLEfrom
christina-roperto:pluginfile_36

Conversation

@christina-roperto

Copy link
Copy Markdown

This PR fixes issue #170 so the static images will go through pluginfile.php and can be cached for a longer period of time.

Comment thread lib.php
if (!in_array($exporttype, ['csv','json', 'ods', 'xls']) ){
send_file_not_found();
}
$filename = $CFG->dirroot.'/blocks/configurable_reports/'.$filearea.'/'.$exporttype.'/pix.gif';

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This one doesn't feel right, shouldn't this just be normal pix serving which ends up via /theme/image.php/xxx ?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hey @brendanheywood - I tried using theme url, but that doesn't work in pluginfile.
In UI, the image URL is showing like it is coming from pluginfile. I hope this is fine.
image

Comment thread managereport.php
$url = moodle_url::make_pluginfile_url($cid, 'block_configurable_reports', 'export', null, '/', $e);
$download .= '<a href="viewreport.php?id='.$r->id.'&amp;download=1&amp;format='.$e.'">'.
'<img src="'.$CFG->wwwroot.'/blocks/configurable_reports/export/'.$e.'/pix.gif" alt="'.$e.'">'.
'<img src="'.$url.'" alt="'.$e.'">'.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

please convert this to html_writer

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hey @brendanheywood - this has been fixed in another pr - #204 (i couldn't push anything to here). Please review.

Thanks,
Anu

Comment thread report.class.php
$context = \context_system::instance();
$cid = $context->id;
$url = moodle_url::make_pluginfile_url($cid, 'block_configurable_reports', 'export', null, '/', $e);
$output .= '<a href="'.$wwwpath.'&amp;download=1&amp;format='.$e.'"><img src="'.$url.'" alt="'.$e.'">&nbsp;'.(strtoupper($e)).'</a>&nbsp;';

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

please convert this to html_writer

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hey @brendanheywood - this has been fixed in another pr - #204 (i couldn't push anything to here). Please review.

Thanks,
Anu

@jleyva jleyva closed this Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants