yireo-joomla / pkg_scriptmerge

GNU General Public License v3.0
7 stars 10 forks source link

Fixes issue with empty lines left after replacement of html tags #19

Closed ghost closed 8 years ago

ghost commented 8 years ago

If compress_html is off, the new scriptmerge css/js tags will leave empty lines when it replaces multiple other script/link tags. This PR fixes that.

PS: Also improves the syntax a bit by having less indentation/nesting of ifs.

jissereitsma commented 8 years ago

Thanks. Good to see you have tracked this down - I've seen those empty lines on my own site a couple of times, and was wondering where my hacks went wrong. For the refactoring part, I thank you and @rdohms.

AlexanderSk commented 8 years ago

Hello guys! I did test the new changes no a couple of live sites and i located a minor issue when one has add size to images enabled. i did track the issue here:

Line 879 private function cleanup($body = null, $matches = array())

on the check if (empty($files)) { continue; }

if it is changed with ! it seems to solve this issue but the function is not working correctly so the issue is somewhere else... if (!empty($files)) { continue; }

I am really busy these days and i cant upload a request ;) Edit: really nice pulls by the way ... be well!! Edit2: the above if (empty($files)) is in fact correct .. the problem is not there.

AlexanderSk commented 8 years ago

Hello again, restoring the old part of code in the cleanup function seams to fix the issue... actually both js and scripts are working along with the image functionality.edit again.. the versioning doesn't work correctly with the old core though. no time to check it further today.

`if ($first) { $replacement = ''; $first = false; } else { $replacement = ''; }

$body = str_replace($file['html'], $replacement, $body);`

ghost commented 8 years ago

Can you explain exactly what isn't working with the new code? What is the difference in html output?

ghost commented 8 years ago

PS: Place code between code characters ` so that it is readable.

AlexanderSk commented 8 years ago

Hello, i was trying to find out why with the new code changes when someone has the backed option add size to images enabled the web site breaks. when i say breaks i mean only html is shown and code... can you confirm that? I faced it just today in two totally different websites.

ghost commented 8 years ago

This code change should not impact that. If it does, please give the concrete html examples I asked for...

AlexanderSk commented 8 years ago

In order to be 100% sure i enabled a testing site i have and upgraded to the latest version. You may see the issue here with the source code. http://demo.digi-web.gr/ joomla version latest - caching is totally off. This is the 3rd site and 3/3 that the issue arises. I hope you might have an idea. Edit: what i can see from the source code is that there are no spaces in between elements eg a href=""> has become ahref=""> whitch might be the source of the problem...

ghost commented 8 years ago

Are you sure this is caused by this pull request?

Can you see if this fixes it? https://github.com/yireo/pkg_scriptmerge/pull/21

PS: I can't reproduce the issue

AlexanderSk commented 8 years ago

Ok i figured it out. latest script merge version 0.10.8 issued yesterday (3-3-2016) causes the issue. The update does not include #21. I can confirm that with the addition of #21 the issue is fixed. I guess you tested the plugin from the repository in order to reproduce it right? So all we have to do is wait for the new version to be released.

AlexanderSk commented 8 years ago

Hello again i located a new issue witch in some cases removes an image from a web site completely.

On Line 928 you added the preg_replace which is my tests seems to be responsible. What is it for if i may ask?

private function cleanup... $body = preg_replace('/\s*' . preg_quote($file['html'], '/') . '/s', '', $body);

as a test case i used again my demo site... please ckeck the url: http://demo.digi-web.gr/%CE%BF%CF%81%CE%B8%CE%BF%CF%80%CE%B1%CE%B9%CE%B4%CE%B9%CE%BA%CE%AC/%CF%80%CE%AD%CE%BB%CE%BC%CE%B1%CF%84%CE%B1/%CF%80%CE%B5%CE%BB%CE%BC%CE%B1%CF%84%CE%BF%CE%B3%CF%81%CE%AC%CF%86%CE%B7%CE%BC%CE%B1-pedcad

The actual code that exists in this page is the following: <p><img src="images/katalogos/pedcad1.jpg" alt="" width="400" height="305" style="margin-left: 10px; float: right;" title="" /></p> <p><strong>ΣΥΓΧΡΟΝΟΣ ΨΗΦΙΑΚΟΣ ΠΕΛΜΑΤΟΓΡΑΦΟΣ </strong></p> <p>Στ</p> As you can see the image is missing and only the <p></p> is visible.

ghost commented 8 years ago

Same question:

Are you sure this is caused by this pull request?

Check if the same thing happen with the version before this PR.

AlexanderSk commented 8 years ago

I am sure about it in the scope that when i comment out the //$body = preg_replace('/\s*' . preg_quote($file['html'], '/') . '/s', '', $body); in the scriptmerge.php file the image is one again visible and everything seems to be working fine. This is why i am asking the purpose of the line since you are the developer.

jissereitsma commented 8 years ago

I can confirm this line of code was added with PR #19. What it does is replace the existing stylesheets and scripts with a space. I'm now adding #21 as well. Let me know if this fixes things.

ghost commented 8 years ago

The code was there before this PR. I'll let @jissereitsma look into this. As he is the developer ;)

ghost commented 8 years ago

The code before this PR did the same:

$replacement = '';
...
$body = str_replace($file['html'], $replacement, $body);

The new code just also removes the whitespace around it