e107inc / e107

e107 Bootstrap CMS (Content Management System) v2 with PHP, MySQL, HTML5, jQuery and Twitter Bootstrap. Issue Discussion Room: https://gitter.im/e107inc/e107
https://e107.org
GNU General Public License v3.0
318 stars 212 forks source link

Navigation icons uploaded with Media Manager used on custom pages are unexpectedly 350×350px #5056

Open Deltik opened 11 months ago

Deltik commented 11 months ago

Bug Description

Due to the reuse of state in e107::getParser(), navigation bar icons that were uploaded to Media Manager will use the same thumbnail dimensions as the ones on cpages.

Using e107 v2.3.2, this is the stack where the thumbnail dimensions are specified:

page_template.php:98, include_once()
e107_class.php:3603, e107::_getTemplate()
e107_class.php:3318, e107::getCoreTemplate()
page.php:657, pageClass->processViewPage()
page.php:71, include()
index.php:107, {main}()

The thumbnail dimensions are set in this stack:

e_parse_class.php:2259, e_parse->thumbCrop()
setimage.php:14, setimage_shortcode()
shortcode_handler.php:1312, e_parse_shortcode->doCode()
shortcode_handler.php:1020, preg_replace_callback()
shortcode_handler.php:1020, e_parse_shortcode->parseCodes()
e_parse_class.php:848, e_parse->parseTemplate()
form_handler.php:4789, e_form->renderRelated()
page_shortcodes.php:760, cpage_shortcodes->sc_cpagerelated()
shortcode_handler.php:1154, e_parse_shortcode->doCode()
shortcode_handler.php:1020, preg_replace_callback()
shortcode_handler.php:1020, e_parse_shortcode->parseCodes()
e_parse_class.php:848, e_parse->parseTemplate()
page.php:933, pageClass->renderPage()
page.php:860, pageClass->setPage()
page.php:72, include()
index.php:107, {main}()

And navigation icons uploaded to e_MEDIA_IMAGE reuse the dimensions here:

e_parse_class.php:4417, e_parse->toIcon()
navigation_shortcodes.php:266, navigation_shortcodes->sc_nav_link_icon()
shortcode_handler.php:1154, e_parse_shortcode->doCode()
shortcode_handler.php:1020, preg_replace_callback()
shortcode_handler.php:1020, e_parse_shortcode->parseCodes()
e_parse_class.php:848, e_parse->parseTemplate()
navigation_shortcodes.php:330, navigation_shortcodes->sc_nav_link_sub()
shortcode_handler.php:1154, e_parse_shortcode->doCode()
shortcode_handler.php:1020, preg_replace_callback()
shortcode_handler.php:1020, e_parse_shortcode->parseCodes()
e_parse_class.php:848, e_parse->parseTemplate()
sitelinks_class.php:1672, e_navigation->render()
navigation.php:52, navigation_shortcode()
shortcode_handler.php:1312, e_parse_shortcode->doCode()
shortcode_handler.php:1020, preg_replace_callback()
shortcode_handler.php:1020, e_parse_shortcode->parseCodes()
e_parse_class.php:848, e_parse->parseTemplate()
e107_class.php:383, e107::renderLayout()
header_default.php:823, require_once()
page.php:80, include()
index.php:107, {main}()

How to Reproduce

Steps to reproduce the behavior:

  1. Upload a small image to Media Manager.
  2. Use it as a navigation item icon.
  3. Navigate to the custom page.
  4. Observe that the icon is too big.

Expected Behavior

image

Actual Behavior

image

Deltik commented 11 months ago

This patch fixes the symptom of this bug, but I'm concerned about potential side effects:

diff --git a/e107_core/shortcodes/single/navigation.php b/e107_core/shortcodes/single/navigation.php
--- a/e107_core/shortcodes/single/navigation.php    (revision a6e1c0b897a3bf3f3e7bd6ea0382e2556e456379)
+++ b/e107_core/shortcodes/single/navigation.php    (date 1691073309731)
@@ -47,6 +47,8 @@
    $template       = e107::getCoreTemplate('navigation', $tmpl);   
    $data           = $nav->initData($category, $parm);

+   e107::getScParser()->parseCodes("{SETIMAGE: default}");
+
    return $nav->render($data, $template, $parm);

 }
Jimmi08 commented 11 months ago

Maybe related: https://github.com/e107inc/e107/issues/4846

Jimmi08 commented 11 months ago

Maybe related: https://github.com/e107inc/e107/issues/4143 See answer.

Jimmi08 commented 11 months ago

A similar answer here: https://github.com/e107inc/e107/issues/2676

CaMer0n commented 9 months ago

This patch fixes the symptom of this bug, but I'm concerned about potential side effects:

diff --git a/e107_core/shortcodes/single/navigation.php b/e107_core/shortcodes/single/navigation.php
--- a/e107_core/shortcodes/single/navigation.php  (revision a6e1c0b897a3bf3f3e7bd6ea0382e2556e456379)
+++ b/e107_core/shortcodes/single/navigation.php  (date 1691073309731)
@@ -47,6 +47,8 @@
  $template       = e107::getCoreTemplate('navigation', $tmpl);   
  $data           = $nav->initData($category, $parm);

+ e107::getScParser()->parseCodes("{SETIMAGE: default}");
+
  return $nav->render($data, $template, $parm);

 }

Perhaps adding {SETIMAGE: default} to the core navigation template is a better idea, at least it would provide theme developers with a way to override this change.

Deltik commented 9 months ago

The issue is the reuse of the e_parse state for a different scope (first in "navigation", then in "page"). The ideal fix would be to initialize the state for that particular scope rather than share one across the entire execution. Otherwise, we're forcing themes to work around quirks/bugs from the e107 core.