Joomla! Issue Tracker | Joomla! CMS #44645 - Update ContenthistoryHelper to use absolute image paths

Updates Requested bug PR-5.3-dev

Pending
Medium
Build: 5.3-dev
# 44645
Diff
Attila-SWE:patch-1

Pending

User tests: Successful: Unsuccessful:

Attila-SWE
20 Dec 2024

My first PR so sorry if I messed something up. Please give me feedback then. Some considerations is that this only fixes images in images/ folder and does not fix other relative URL:s or such. Perhaps a broader fix is necessary?

Pull Request for Issue #44635.

Summary of Changes

Check to see if values in content history includes images with relative /images url

Testing Instructions

Open preview of content history with images in content with relative urls like images/test.png

Actual result BEFORE applying this Pull Request

Images broken because urls are relative to /administrator/ so for example DOMAIN/administrator/images/test.png

Expected result AFTER applying this Pull Request

Images shown and url of images is DOMAIN/images/test.png

Link to documentations

No documentation changes for docs.joomla.org needed
No documentation changes for manual.joomla.org needed

a27b437 20 Dec 2024

Update ContenthistoryHelper to use absolute image paths

Attila-SWE - open - 20 Dec 2024

Attila-SWE - change - 20 Dec 2024

Status

New

⇒

Pending

joomla-cms-bot - change - 20 Dec 2024

Category

⇒

Administration com_content com_contenthistory

Attila-SWE - change - 20 Dec 2024

The description was changed

Attila-SWE - edited - 20 Dec 2024

fgsw - comment - 20 Dec 2024

@Attila-SWE Thanks for your first PR. Please close the issue.

I will test the PR but wan't to wait first if there is Feedback.

Attila-SWE - comment - 20 Dec 2024

Done! So if I understand correctly then first create issue, then create pull request referring to issue and close issue? Is there any "documentation" for this regarding Joomla that I could read up on?

fgsw - comment - 20 Dec 2024

If you create a PR no Issue is needed.

I don't know about Documentation for this workflow.

a05f6a5 20 Dec 2024

Updated PR with no deprecated classes...

Attila-SWE - change - 20 Dec 2024

Labels

Added: PR-5.3-dev

fgsw - comment - 21 Dec 2024

@alikon Makes it sense to test the Pull Request as continuous-integration/drone/pr — Build is failing?

alikon - comment - 21 Dec 2024

@fgsw drone is failing at Code style step

59373e0 21 Dec 2024

Changed order use-list

Attila-SWE - comment - 21 Dec 2024

NP! Thanks for helping me learn

26793f3 21 Dec 2024

Change to global \is_string() instead of is_string()

e4751f7 21 Dec 2024

code style

ecd0668 22 Dec 2024

Merge pull request #1 from alikon/patch-18

fgsw - test_item - 23 Dec 2024 - Tested successfully

fgsw - comment - 23 Dec 2024

I have tested this item ✅ successfully on ecd0668

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/44645.}

chmst - comment - 25 Dec 2024

Thank you very much @Attila-SWE and congratulations to your first PR!

alikon - comment - 31 Dec 2024

successfully tested by @Quy on ecd0668

alikon - alter_testresult - 31 Dec 2024 - Quy: Tested successfully

alikon - change - 31 Dec 2024

Status

Pending

⇒

Ready to Commit

alikon - comment - 31 Dec 2024

RTC

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/44645.}

Fedik - change - 31 Dec 2024

Status

Ready to Commit

⇒

Pending

Fedik - comment - 31 Dec 2024

Back to Pending status.

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/44645.}

Fedik - comment - 31 Dec 2024

I am sorry, the changes will not going to work reliable. But thanks for your try.
You is assuming that all images are from images/ folder, however it not always true.

A solid fix going to be much more complex. It need something similar to what we have in SEF plugin

joomla-cms/plugins/system/sef/src/Extension/Sef.php

Lines 229 to 259 in 9800738

    
           // Check for all unknown protocols (a protocol must contain at least one alphanumeric character followed by a ":"). 
        
           $protocols  = '[a-zA-Z0-9\-]+:'; 
        
           $attributes = ['href=', 'src=', 'poster=']; 
        
           foreach ($attributes as $attribute) { 
        
               if (strpos($buffer, $attribute) !== false) { 
        
                   $regex  = '#\s' . $attribute . '"(?!/|' . $protocols . '|\#|\')([^"]*)"#m'; 
        
                   $buffer = preg_replace($regex, ' ' . $attribute . '"' . $base . '$1"', $buffer); 
        
                   $this->checkBuffer($buffer); 
        
               } 
        
           } 
        
           if (strpos($buffer, 'srcset=') !== false) { 
        
               $regex = '#\s+srcset="([^"]+)"#m'; 
        
               $buffer = preg_replace_callback( 
        
                   $regex, 
        
                   function ($match) use ($base, $protocols) { 
        
                       preg_match_all('#(?:[^\s]+)\s*(?:[\d\.]+[wx])?(?:\,\s*)?#i', $match[1], $matches); 
        
                       foreach ($matches[0] as &$src) { 
        
                           $src = preg_replace('#^(?!/|' . $protocols . '|\#|\')(.+)#', $base . '$1', $src); 
        
                       } 
        
                       return ' srcset="' . implode($matches[0]) . '"'; 
        
                   }, 
        
                   $buffer 
        
               ); 
        
               $this->checkBuffer($buffer); 
        
           }

4a9f573 9 Jan 2025

Universal absolute path

Attila-SWE - change - 9 Jan 2025

Labels

Added: Updates Requested bug

Attila-SWE - comment - 9 Jan 2025

New suggested solution.

Fedik - comment - 9 Jan 2025

Thanks. That looks much better. Still few things:

Check that the string actualy contain src= before running reg expression (as it done in SEF plugin). Can use str_contains()
You checking only https, but we should check any protocol (as it done in SEF plugin)
Code style: https://ci.joomla.org/joomla/joomla-cms/81227/1/7

Attila-SWE - comment - 9 Jan 2025

Should we also fix all relative 'href' or just 'src'?

Fedik - comment - 10 Jan 2025

If you like you can do for href also, sure.

Add a Comment

Older
Newer

Joomla! Issue Tracker - CMS

[#44645] - Update ContenthistoryHelper to use absolute image paths

Summary of Changes

Testing Instructions

Actual result BEFORE applying this Pull Request

Expected result AFTER applying this Pull Request

Link to documentations

Add a Comment

	// Check for all unknown protocols (a protocol must contain at least one alphanumeric character followed by a ":").
	$protocols = '[a-zA-Z0-9\-]+:';
	$attributes = ['href=', 'src=', 'poster='];

	foreach ($attributes as $attribute) {
	if (strpos($buffer, $attribute) !== false) {
	$regex = '#\s' . $attribute . '"(?!/\|' . $protocols . '\|\#\|\')([^"]*)"#m';
	$buffer = preg_replace($regex, ' ' . $attribute . '"' . $base . '$1"', $buffer);
	$this->checkBuffer($buffer);
	}
	}

	if (strpos($buffer, 'srcset=') !== false) {
	$regex = '#\s+srcset="([^"]+)"#m';

	$buffer = preg_replace_callback(
	$regex,
	function ($match) use ($base, $protocols) {
	preg_match_all('#(?:[^\s]+)\s(?:[\d\.]+[wx])?(?:\,\s)?#i', $match[1], $matches);

	foreach ($matches[0] as &$src) {
	$src = preg_replace('#^(?!/\|' . $protocols . '\|\#\|\')(.+)#', $base . '$1', $src);
	}

	return ' srcset="' . implode($matches[0]) . '"';
	},
	$buffer
	);

	$this->checkBuffer($buffer);
	}