Download menu plugin + some issues with download plugin

[Jimmi08]

We have released a small community plugin that adds a latest downloads menu, a top downloads menu and an admin view of the download_requests log to the core Downloads plugin. It adds no tables and no columns — the request log it reports on already ships with core, it just has no UI.

REPO: https://github.com/Jimako-e107-plugins/download_extended

This post is informational rather than a bug report. Two things came up while testing it that maintainers may want to know about. We may be wrong about either — please read them as observations.

First: the Downloads index can build its breadcrumb from stale shortcode data

In e107_plugins/download/handlers/download_class.php, renderCategories() asks the shortcode batch for the breadcrumb before it hands over the query state:

$sc = e107::getScBatch('download', true);
$sc->wrapper('download/categories');
$sc->breadcrumb();          // line 314
$sc->qry = $this->qry;      // line 315

download_shortcodes::breadcrumb() switches on varset($this->qry['action']), which is still unset at that point. The index therefore takes the default branch instead of case 'maincats', and the default branch reads $this->var. The same ordering appears in loadView() (lines 522-524), where it is masked because renderView() calls breadcrumb() again once the vars are set.

On a stock install nothing is visible: $this->var is empty, the default branch produces one crumb with an empty text, and form_handler::breadcrumb() drops empty entries. It only becomes visible once something else on the page has put a row into the same batch — which is what our menus did. The Downloads index then showed a breadcrumb for a file that is not on that page.

Swapping the two lines seems to be enough: case 'maincats' never touches $this->var, so the index becomes independent of whatever state the batch is in.

Second: the shortcode batch is shared for the whole request

e107::getScBatch('download', true) returns a cached instance, and setVars() is a plain assignment ($this->var = $eVars;). Anything that renders before the page controller — a menu, for instance — leaves its last row in the batch, and the controller inherits it.

We fixed this on our side by saving and restoring the batch state around the render loop, and that alone makes our plugin well-behaved. We mention it only because the first point is what makes the leak visible, and a third-party plugin is the most likely way anyone will hit it.

On verification

The site where this was reproduced runs a 2.4-based fork, so we checked the plugin first. handlers/download_class.php and download_shortcodes.php are byte-identical to upstream master; the only local differences in the Downloads plugin are in e_url.php and request.php, neither of which touches the breadcrumb path. So the behaviour described above comes from upstream code, not from a local modification.

---

Third: A side observation on class filtering

The Downloads plugin's own listings filter on download_visible and download_category_class only — renderList() (lines 850-859) and loadView() (lines 540-541). Its addons filter on download_class as well: e_list.php:48, e_rss.php:104, e_gsitemap.php:48, e_search.php:62, e_tagwords.php:38. Both behaviours are defensible, but a plugin author reading the source has no way to tell which one is intended for a new listing. We went with the plugin's own pages, on the grounds that a site menu is closer to an on-site listing than to a feed.

---

Line references — all verified against e107inc/e107 master, 2026-09-19

claim	file	line
$sc->breadcrumb() before $sc->qry	download/handlers/download_class.php	314-315
same ordering, masked	download/handlers/download_class.php	522-524
breadcrumb() switches on $this->qry['action']	download/download_shortcodes.php	81-125
setVars() is a plain assignment	e107_handlers/shortcode_handler.php	1886
getVars() / setVars() are public	e107_handlers/shortcode_handler.php	1940 / 1898
empty crumbs dropped	e107_handlers/form_handler.php	breadcrumb()
renderList() file query — no download_class	download/handlers/download_class.php	850-859
loadView() query — no download_class	download/handlers/download_class.php	540-541
addons that do filter download_class	download/e_list.php	48
	download/e_rss.php	104
	download/e_gsitemap.php	48
	download/e_search.php	62
	download/e_tagwords.php	38

[e107help[bot]]

Ahoj Jana,

All three points check out, @Jimmi08, and the first is worse than you found: the same ordering breaks the mirror page with no third-party plugin involved. Two tickets out of it, #6586 and #6587. Every line number in your write-up matched master.

1. The breadcrumb ordering. Traced and confirmed on master and release/v2.3.x. Swapping the two lines is the fix, exactly as you said. renderMirror() has the same ordering and nothing masks it there, so case 'mirror' is unreachable and the page prints one Downloads crumb instead of Downloads / Category / File / Select mirror .... Details in #6586.

2. The shared batch. Confirmed in the code: getScObject() stores the instance and hands the same one back for the rest of the request.

e107/e107_handlers/shortcode_handler.php

Lines 451 to 464 in 483b4bc

	// Includes global Shortcode Classes (e_shortcode.php) or already loaded batch
	if ($this->isScClass($className))
	{
	return $this->scClasses[$className];
	}
	// If it already exists - don't include it again.
	if (class_exists($className, false)) // don't allow __autoload()
	{
	// $this->registerClassMethods($className, $path); // XXX Global registration should happen separately - here we want only the object.
	$this->scClasses[$className] = new $className();
	return $this->scClasses[$className];

Saving and restoring around your render loop is the right move for a plugin, and getVars()/setVars() are public so you can keep relying on them. The caching does not need to change for the index to behave; the ordering in point 1 is what makes the index independent of whatever state the batch is in.

3. download_visible versus download_class. A site menu that follows the plugin's own pages is consistent, so your choice stands. The difference between the two fields is where each one is enforced: the plugin's listings decide what to show from download_visible, while download_class is checked when the file is actually fetched.

e107/e107_plugins/download/request.php

Lines 503 to 508 in 483b4bc

	private static function refuse_hidden_download($row)
	{
	if(!check_class(varset($row['download_category_class'], 0)) || !check_class($row['download_class']))
	{
	e107::redirect(e107::url('download', 'index', null, array('query' => array('action' => 'error', 'id' => 1))));
	exit();

Which is why you found two behaviours rather than a mistake. The addons are the stricter side, and that costs something real: the same file can be listed on-site and missing from the feed, the sitemap and the search results for the same visitor. Both are defensible, as you said, and core has never chosen between them.

The second ticket, #6587, came out of the same read of the mirror page: it fetches its download row with no active, visible or class condition at all, and its per-mirror download button targets the wrong script.

Thanks for the plugin, and for a report I could follow straight into the code.

[e107help[bot]]

@Jimmi08 the breadcrumb ordering is fixed and up for review: #6588 for master, #6589 for release/v2.3.x. Merging is Deltik's call. Neither one asks you to drop the save and restore around your render loop.

What a theme sees afterwards: the Downloads index prints one crumb, Downloads, whatever another render left in the batch before it, and the mirror page prints the four it always meant to (Downloads / category / file / Select mirror ...) where it printed one. The item page's markup does not change at all.

One thing came out of writing the tests rather than out of your report, and it sits nearer your side of the fence: a theme that overrides download_template.php has to carry a categories key of its own. Core does not merge its own template in behind an override, so without that key parent, child and subchild arrive empty and every category row is parsed from nothing. #6588 guards those reads, and caption with them, which stops PHP 8 printing warnings over the page. It does not make the page render: that still needs the key. release/v2.3.x has had the guards all along.