[2.x] Fix bugs and code improvements #111

joaorobertopb · 2019-09-12T03:01:35Z

Description

This big PR will:

Drop support for Laravel <5.5;
Bump PHP minimun version to 7.1.3+;
Update dependency constraints in composer.json;
Recfator all tests:
- Remove deprecated functions e prepare code to next phpunit 9.0 version;
- Improvements tests coverage;
Fix RemoveComments middleware, now it remove JS and CSS comments;
Fix CollapseWhitespace, now dependent on 'RemoveComments' middleware;
Update the old phpunit.xml configuration file;
Update the .travis.yml file and run the tests against in PHP 7.3 / PHP 7.4;
Update README.md;

Motivation and context

Why is this change required? What problem does it solve?

It fixes: #30 #46 #55 #57 #77 #82 #107 #108

The CollapseWriteSpace::class filter was producing a bug when the HTML contains inline JS comments.... All JS code after the inline comment was also commented!

For example:

<!DOCTYPE html>
<html lang="en">
    <head>
        <meta charset="utf-8">
        <meta http-equiv="x-ua-compatible" content="ie=edge">
        <title></title>
        <meta name="description" content="">
    </head>
    <body>
        <script>
            // Alert Hello!!
            alert('Hello!!');

            // Log Hello!!
            console.log('Hello!!');
        </script>
    </body>
</html>

After filter apply:

<!DOCTYPE html><html lang="en"><head><meta charset="utf-8"><meta http-equiv="x-ua-compatible" content="ie=edge"><title></title><meta name="description" content=""></head><body><script> // Alert Hello!! alert('Hello!!'); // Log Hello!! console.log('Hello!!'); </script></body></html>

What is the solution?

We have to remove all inline JS comments before the 'CollapseWriteSpace' filter is executed.

🤔 ... Okay, we need to make sure RemoveComments::class filter always runs before CollapseWriteSpace::class filter!

All right, it's done:

laravel-page-speed/src/Middleware/CollapseWhitespace.php

Lines 5 to 25 in efc2f37

    
           class CollapseWhitespace extends PageSpeed 
        
           { 
        
               public function apply($buffer) 
        
               { 
        
                   $replace = [ 
        
                       "/\n([\S])/" => '$1', 
        
                       "/\r/" => '', 
        
                       "/\n/" => '', 
        
                       "/\t/" => '', 
        
                       "/ +/" => ' ', 
        
                       "/> +</" => '><', 
        
                   ]; 
        
                   return $this->replace($replace, $this->removeComments($buffer)); 
        
               } 
        
               protected function removeComments($buffer) 
        
               { 
        
                  return (new RemoveComments)->apply($buffer); 
        
               } 
        
           }

But...

Oooh Nooo! The RemoveComments::class filter was only removing HTML comments 🤦‍♂️...

Not problem ... RemoveComments::class has been updated! Now, it removes HTML, JS and CSS comments:

laravel-page-speed/src/Middleware/RemoveComments.php

Lines 5 to 16 in 332cbf6

    
           class RemoveComments extends PageSpeed 
        
           { 
        
               public function apply($buffer) 
        
               { 
        
                   $replace = [ 
        
                       '/<!--[^]><!\[](.*?)[^\]]-->/s' => '', 
        
                       '/(?:(?:\/\*(?:[^*]|(?:\*+[^*\/]))*\*+\/)|(?:(?<!\:|\\\|\'|\")\/\/.*))/' => '', 
        
                   ]; 
        
                   return $this->replace($replace, $buffer); 
        
               } 
        
           }

How has this been tested?

Refactoring and improve test coverage.

Types of changes

What types of changes does your code introduce? Put an x in all the boxes that apply:

Bugfix / Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

I have read the CONTRIBUTING document.
My pull request addresses exactly one patch/feature.
I have created a branch for this patch/feature.
Each individual commit in the pull request is meaningful.
I have added tests to cover my changes.
If my change requires a change to the documentation, I have updated it accordingly.

I did a lot of testing and it seems to be working okay...

Can someone help me to test this PR, please? 😄

Ping @renatomarinho

muarachmann · 2019-09-14T13:42:25Z

@joaorobertopb thanks will test and get back to you

muarachmann · 2019-09-14T13:43:45Z

Also, i noticed that the Urls were truncated when the application is served
for example instead of https://www.google.com it shows //www.google.com but still loads the library.

joaorobertopb · 2019-09-14T17:06:44Z

@muarachmann yes man, it is correct.

See:

\RenatoMarinho\LaravelPageSpeed\Middleware\TrimUrls::class,

The TrimUrls::class filter trims URLs by resolving them by making them relative to the base URL for the page.

Warning: TrimUrls::class is considered medium risk. It can cause problems if it uses the wrong base URL. This can happen, for example, if you serve HTML that will be pasted verbatim into other HTML pages. If URLs are trimmed on the first page, they will be incorrect for the page they are inserted into. In this case, just disable the middleware.

HDVinnie · 2019-09-14T17:21:21Z

@joaorobertopb beautiful PR. Very clean and documented. Tested and cannot appear to find any issues yet. Will report back if do find something this weekend.

muarachmann · 2019-09-15T21:31:16Z

Hi @joaorobertopb tested on a live laravel (5.8) application and it seems to be breaking on the <img src=. I removed this and it breaks on the <script src= tag. Here is a sample of the page source

For example here is a sample page and it seems to break on line 9 https://gist.github.com/muarachmann/a020549549e332c8304a5cecce270c14 as seen below.

This is when i include all the stuffs in the Kernel.php removing them one by one to see the pb
I see it is with the \RenatoMarinho\LaravelPageSpeed\Middleware\RemoveComments::class

	     // Laravel Page Speed
	    \RenatoMarinho\LaravelPageSpeed\Middleware\InlineCss::class,
	    \RenatoMarinho\LaravelPageSpeed\Middleware\ElideAttributes::class,
	    \RenatoMarinho\LaravelPageSpeed\Middleware\InsertDNSPrefetch::class,
	  //  \RenatoMarinho\LaravelPageSpeed\Middleware\RemoveComments::class,
	    \RenatoMarinho\LaravelPageSpeed\Middleware\TrimUrls::class,
	    \RenatoMarinho\LaravelPageSpeed\Middleware\RemoveQuotes::class,
	    \RenatoMarinho\LaravelPageSpeed\Middleware\CollapseWhitespace::class,

Not sure of what is happening as i instead suspected the TrimUrls to behave as such.
If not removing that everything else works fine :)

muarachmann · 2019-09-16T02:04:37Z

And also just noticed my laravel debug screen shows blank without the usual laravel debug screen if an error occurs. It just returns a 500 error

joaorobertopb · 2019-09-17T00:32:07Z

Hi @muarachmann, thanks for reporting this.

I'll check!

vineetha-123 · 2019-09-19T06:40:51Z

how to integrate above new code of RemoveComments into my project

joaorobertopb · 2019-09-20T01:24:43Z

@vineetha-123 Would have to wait for this PR to be merged.

OR

You can configure composer.json to require my fork repository. This is not very recommended, but if you are in a hurry ...

{
    "repositories": [
        {
            "type": "vcs",
            "url": "https://github.com/joaorobertopb/laravel-page-speed"
        }
    ],
    "require": {
        "renatomarinho/laravel-page-speed": "dev-fix-bugs-and-code-improvements"
    }
}

See more details: https://stackoverflow.com/questions/13498519/how-to-require-a-fork-with-composer

vineetha-123 · 2019-09-20T09:04:23Z

@vineetha-123 Would have to wait for this PR to be merged.

OR

You can configure composer.json to require my fork repository. This is not very recommended, but if you are in a hurry ...
{
    "repositories": [
        {
            "type": "vcs",
            "url": "https://github.com/joaorobertopb/laravel-page-speed"
        }
    ],
    "require": {
        "renatomarinho/laravel-page-speed": "dev-fix-bugs-and-code-improvements"
    }
}
See more details: https://stackoverflow.com/questions/13498519/how-to-require-a-fork-with-composer

thanks

Cannonb4ll · 2019-11-01T14:07:07Z

Is there any ETA when this will be merged and released?

joaorobertopb · 2019-11-26T11:22:23Z

Sorry for the delay 😅 ! It will be merged later this year...

deekumar24 · 2019-12-26T06:00:41Z

I don't know if I'm 100% right here,
if you're using TrimUrls, it'll trim the Laravel's assets URL and the regex is taking that line as comment.
I tried to use it in double quotes. but its still appearing that way.
See the screenshot below.

joaorobertopb · 2019-12-28T15:26:49Z

@rudee24 thanks for reporting this.

I'll check!

huyvl102 · 2020-01-13T03:02:46Z

2020 coming :D , Did u updated 💃

lucasMesquitaBorges

Thanks @joaorobertopb
I have tested this PR a lot and I just found one problem with RemoveComments middleware.
It's breaking html code for "//" strings.

src/Middleware/RemoveComments.php

muarachmann · 2020-02-13T13:35:35Z

That would be awesome! man looking forward to this

On Thu, 13 Feb 2020 at 13:35 Lucas Mesquita Borges ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/Middleware/RemoveComments.php <#111 (comment)> : > @@ -7,7 +7,8 @@ class RemoveComments extends PageSpeed public function apply($buffer) { $replace = [ - '//s' => '' + '//s' => '', + '/(?:(?:\/\*(?:[^*]|(?:\*+[^*\/]))*\*+\/)|(?:(?<!\:|\\\|\'|\")\/\/.*))/' => '', Hi @joaorobertopb <https://github.com/joaorobertopb> I believe i'm finishing this fix. If everything goes right, this afternoon I'll push my commit fixing this issue. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#111?email_source=notifications&email_token=AD4X4XQHS32CSHN26OCV7WDRCU5BLA5CNFSM4IV54FQ2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCVMWRUA#discussion_r378832940>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AD4X4XVWJCDNH2ISYK7RYG3RCU5BLANCNFSM4IV54FQQ> .

-- *MUA N. LAURENT*: Software Engineer Akivas Inc. <https://akivas.com/> Akwa, Douala,CM 00237 | 174 Royal Rd, Cape Town, WC 7405, SA Phone: (237) 670-518-086

gagandeep995 · 2020-05-15T15:16:58Z

any updates about the release?

lucasMesquitaBorges · 2020-05-15T15:56:27Z

Yes @gagandeep995! We're currently working on the milestone 2.0.0 that will address this release.

joaorobertopb · 2020-05-16T16:00:09Z

Merging this PR in branch 2.x

joaorobertopb mentioned this pull request Sep 20, 2019

single line comments in js ans css commenting remain code #112

Closed

joaorobertopb force-pushed the fix-bugs-and-code-improvements branch from 1e063e4 to 790a716 Compare October 20, 2019 17:56

joaorobertopb mentioned this pull request Nov 26, 2019

Ignition not working after installing this package. #116

Closed

joaorobertopb mentioned this pull request Dec 28, 2019

[2.x] Implementation of defer_javascript #117

Merged

lucasMesquitaBorges reviewed Jan 30, 2020

View reviewed changes

src/Middleware/RemoveComments.php Show resolved Hide resolved

lucasMesquitaBorges mentioned this pull request Feb 15, 2020

Fix remove comments html context joaorobertopb/laravel-page-speed#1

Closed

10 tasks

joaorobertopb mentioned this pull request Mar 5, 2020

Added Laravel 6 & 7 Support #120

Closed

joaorobertopb changed the title ~~Fix bugs and code improvements~~ [2.x] Fix bugs and code improvements May 16, 2020

joaorobertopb changed the base branch from master to 2.x May 16, 2020 14:11

joaorobertopb added 13 commits May 16, 2020 12:19

Fix remove JS and CSS comments

e112b66

Improve test coverage

e83583a

'CollapseWhitespace' dependent on 'RemoveComments' middleware

10002bb

Update multi-line comment

2b8531b

Update phpunit config

dc8721b

Drop Laravel <5.4 support && Update dependency constraints

28622b4

Update .gitignore

90a9ed2

Refactoring tests

df179bd

Run tests against PHP 7.4

8b0a734

Update README.md

97e9bbf

Remove unused middlewares

37437f9

Update .travis.yml

9f0f95f

Update constraints

8794aaf

joaorobertopb force-pushed the fix-bugs-and-code-improvements branch from 8dc9125 to 8794aaf Compare May 16, 2020 15:33

joaorobertopb merged commit 8bae9d0 into renatomarinho:2.x May 16, 2020

lucasMesquitaBorges mentioned this pull request May 16, 2020

[2.x] Fix remove comments html context #126

Merged

10 tasks

joaorobertopb deleted the fix-bugs-and-code-improvements branch May 16, 2020 17:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[2.x] Fix bugs and code improvements #111

[2.x] Fix bugs and code improvements #111

joaorobertopb commented Sep 12, 2019 •

edited

muarachmann commented Sep 14, 2019

muarachmann commented Sep 14, 2019

joaorobertopb commented Sep 14, 2019

HDVinnie commented Sep 14, 2019

muarachmann commented Sep 15, 2019

muarachmann commented Sep 16, 2019

joaorobertopb commented Sep 17, 2019

vineetha-123 commented Sep 19, 2019

joaorobertopb commented Sep 20, 2019

vineetha-123 commented Sep 20, 2019

Cannonb4ll commented Nov 1, 2019

joaorobertopb commented Nov 26, 2019

deekumar24 commented Dec 26, 2019

joaorobertopb commented Dec 28, 2019

huyvl102 commented Jan 13, 2020

lucasMesquitaBorges left a comment •

edited

muarachmann commented Feb 13, 2020 via email

gagandeep995 commented May 15, 2020

lucasMesquitaBorges commented May 15, 2020

joaorobertopb commented May 16, 2020

	class CollapseWhitespace extends PageSpeed
	{
	public function apply($buffer)
	{
	$replace = [
	"/\n([\S])/" => '$1',
	"/\r/" => '',
	"/\n/" => '',
	"/\t/" => '',
	"/ +/" => ' ',
	"/> +</" => '><',
	];

	return $this->replace($replace, $this->removeComments($buffer));
	}

	protected function removeComments($buffer)
	{
	return (new RemoveComments)->apply($buffer);
	}
	}

	class RemoveComments extends PageSpeed
	{
	public function apply($buffer)
	{
	$replace = [
	'/<!--[^]><!\[](.*?)[^\]]-->/s' => '',
	'/(?:(?:\/\(?:[^]\|(?:\+[^\/]))\+\/)\|(?:(?<!\:\|\\\\|\'\|\")\/\/.*))/' => '',
	];

	return $this->replace($replace, $buffer);
	}
	}

[2.x] Fix bugs and code improvements #111

[2.x] Fix bugs and code improvements #111

Conversation

joaorobertopb commented Sep 12, 2019 • edited

Description

Motivation and context

What is the solution?

But...

How has this been tested?

Types of changes

Checklist:

muarachmann commented Sep 14, 2019

muarachmann commented Sep 14, 2019

joaorobertopb commented Sep 14, 2019

\RenatoMarinho\LaravelPageSpeed\Middleware\TrimUrls::class,

HDVinnie commented Sep 14, 2019

muarachmann commented Sep 15, 2019

muarachmann commented Sep 16, 2019

joaorobertopb commented Sep 17, 2019

vineetha-123 commented Sep 19, 2019

joaorobertopb commented Sep 20, 2019

vineetha-123 commented Sep 20, 2019

Cannonb4ll commented Nov 1, 2019

joaorobertopb commented Nov 26, 2019

deekumar24 commented Dec 26, 2019

joaorobertopb commented Dec 28, 2019

huyvl102 commented Jan 13, 2020

lucasMesquitaBorges left a comment • edited

Choose a reason for hiding this comment

muarachmann commented Feb 13, 2020 via email

gagandeep995 commented May 15, 2020

lucasMesquitaBorges commented May 15, 2020

joaorobertopb commented May 16, 2020

joaorobertopb commented Sep 12, 2019 •

edited

lucasMesquitaBorges left a comment •

edited