jacobbennett / laravel-http2serverpush Goto Github PK

Hey!

I took a look at the code, good job!! However I noticed that resources with type "fetch" are not picked up as the $linkTypeMap array in buildLinkHeaderString() won't find any correspondence with those extensions.

Example:
<link rel="preload" href="/api/events" as="fetch">

Since api calls like that have no extension, would it be possible to set "fetch" as default in case no other file format matches?

Thank you!

Laravel 5.6 requires ~4.0 for symfony/css-selector, while this package requires ^2.7|^3.0.

Using this middleware currently pushes resources on every single page load, which is additional bandwidth, and defeats the purpose of using it in the first place. Although the client may cancel the push request once it determines it already has the content being pushed, but this is after-the-fact i.e. once the content has already started being sent.

It's not possible to know for sure that content has been cached, but it is possible to take a "best guess" approach by setting a cookie on the request.

You can then assume that if the visitor has the cookie, you won't need to add the link headers.

If the visitor doesn't have the cookie, set it and send the link headers, if the visitor does have the cookie, we can assume that the content was already cached on the client side and skip sending the link headers.

The simplest way of doing this would be to set a cookie with an expiration of say one week. The client will handle cases where the CSS has changed during this time by falling back to normal HTTP requests, fetching the updated CSS, and subsequently caching it.

I've submitted the issue here as a placeholder to flag this, but will work on a PR for you to take a look at later on.

Currently I'm working on a multilanguage website where there is already a Link header set to the response:

<https://example.com/nl>; rel="alternate"; hreflang="nl",<https://example.com/en>; rel="alternate"; hreflang="en"

I was thinking about changing the AddHttp2ServerPush::addLinkHeader() method to prepend the Link header if already present:

if ($response->headers->get('Link')) {
    $link .= ',' . $response->headers->get('Link');
}

$response->header('Link', $link);

Of course I'll be able to extend the middleware and override the method. But would it be something to add to the core?

Hello,

The resources should be unique.

example :

[...]rel=preload; as=image,</img/blog-no-thumbnail.png>; rel=preload; as=image,</img/blog-no-thumbnail.png>; rel=preload; as=image,</img/blog-no-thumbnail.png>;[...]

should not happen

I am grateful to you for the package you developed to improve performance, which is Laravel's worst point.

I got a "Premature end of script headers: ea-php71" error in a Laravel 5.5 project running with PHP 7.1 on CENTOS 6.10 virtuozzo (CPanel WHM).

Long research and debug operations at the end; The problem disappeared when I uninstalled the laravel-HTTP2ServerPush package.

In my research, it is stated that there is a problem with the header length sent to the server.

Do I need to make a setting when I use the package or a setting on the apache server?

Thanks, good work!

Since version 3.0.0, links without protocol in the pages are placed in the link header with only one slash instead of two.

E.g.

<link rel="stylesheet" href="//cdn.jsdelivr.net/npm/[email protected]/dist/tingle.min.css">

Was appended to request headers with a single leading slash

...
links: </cdn.jsdelivr.net/npm/[email protected]/dist/tingle.min.css>; rel=preload; as=style,...

Instead of with two leading slashes (as until version 2.0.6)

...
links: <//cdn.jsdelivr.net/npm/[email protected]/dist/tingle.min.css>; rel=preload; as=style,...

In doing so, the asset is considered relative to the site's url rather than absolute as an external resource.

You can see a live demo of the problem here, by opening the development console.

https://staging2.vivi.city/it

This package causes a fatal error on Safari browsers on iOS.

Server is running nginx with http2 enabled.

When using relative URLs, it creates errors.

The patch could be quite simple, something like that :

diff --git a/src/Middleware/AddHttp2ServerPush.php b/src/Middleware/AddHttp2ServerPush.php
index 253cf28..adb94d8 100644
--- a/src/Middleware/AddHttp2ServerPush.php
+++ b/src/Middleware/AddHttp2ServerPush.php
@@ -115,7 +115,9 @@ class AddHttp2ServerPush
         $type = collect($linkTypeMap)->first(function ($type, $extension) use ($url) {
             return str_contains(strtoupper($url), $extension);
         });
-
+        if(!preg_match('%^https?://%i', $url)) {
+            $basePath = config('http2serverpush.base_path', '/');
+            $url = $basePath . ltrim($url, $basePath);
+        }
         return is_null($type) ? null : "<{$url}>; rel=preload; as={$type}";
     }

But it would require a config file...
In my case, base path is always '/'. But it could be any other path...

As Titile.
Is php8 support release?

Hi,

First, thanks for creating this awesome package!

I noticed you already added a check for JSON responses in this commit.

I just bumped onto this JSON problem while I was trying to write some tests for my project...

Symfony\Component\Debug\Exception\FatalThrowableError: Type error: Argument 1 passed to JacobBennett\Http2ServerPush\Middleware\AddHttp2ServerPush::generateAndAttachLinkHeaders() must be an instance of Illuminate\Http\Response, instance of Illuminate\Http\JsonResponse given

Will you be tagging the latest commits in a new release soon?

👍

running
php artisan vendor:publish --provider="JacobBennett\Http2ServerPush\ServiceProvider"
gives me:

Unable to locate publishable resources.
Publishing complete.

Laravel version 7.13.0

It would seem to make sense that this middlware is only needed on the web group in Laravel 5.2 and 5.3 since the API doesn't/shouldn't normally return any assets the browser would need to display. And most API clients do not support HTTP2 Push in the first place. Would it make sense to change the documentation to reflect that fact?

It seems we can not get the latest package from packagist when I'm using Laravel 5.6.
From the log, the latest release is v2.0.4, which is commit 75d9fd6, not ac84d27.
Could you publish a new release to let we use it in Laravel 5.6 project?
Thanks. 😄

See also #21.

Hi,

I tried this package in a Laravel 5.3 app and found that the assets url where compared against the 'type' (style, script, image) in stead of the extension (.CSS, .JS, etc..).

This change fixed it for me:
appstract@724269a

But I don't know if this will play nice with older Laravel versions?

Your packagist repo is not updated to 4.0.2

https://packagist.org/packages/jacobbennett/laravel-http2serverpush

Maybe you have misconfigured the webhook?

Would it work by simply adding the class to kernel or need to configure more. As my push state is not working as it should.

PR #32 can fix this issue.
Exclusion list is always an empty array, even if your config file is not.

I applied the latest pull request in this repo into my own fork.
Feel free to use that version if you want. https://github.com/VSG24/laravel-HTTP2ServerPush

not sure what exactly is the issue but for the pages that have images in it (mostly jpg) i get a nginx 502 error.

i tried the put the middleware in both middleware & middlewareGroups and both give the same error.

if u need any extra info plz do tell me.