Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix trailing slash #179

Merged
merged 3 commits into from
Aug 2, 2019
Merged

Fix trailing slash #179

merged 3 commits into from
Aug 2, 2019

Conversation

nuvoPoint
Copy link

Fixes the issue where a site has a trailing slash in the home_url.

Fixes the issue where a site has a trailing slash in the home_url.
@@ -119,7 +119,7 @@ public function get_theme_color() {
public function get_manifest() {
$manifest = array(
'name' => wp_kses_decode_entities( get_bloginfo( 'name' ) ),
'start_url' => get_home_url(),
'start_url' => user_trailingslashit( get_home_url() ),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be trailingslashit()?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tested it with and without a trailing slash in the home url and as I understand user_trailingslashit documentation it will check if the permalink settings has a trailing slash.

"Conditionally adds a trailing slash if the permalink structure has a trailing slash, strips the trailing slash if not."

https://codex.wordpress.org/Function_Reference/user_trailingslashit

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about instead just doing.

'start_url' => home_url( '/' ),

This is what core does to get the URL to the front page: https://github.com/WordPress/wordpress-develop/blob/a8a4c09f33356475f4a3f3a9f5bdacd0c25bcb42/src/wp-includes/link-template.php#L332-L333

@westonruter
Copy link
Collaborator

What is the underlying issue being fixed?

@nuvoPoint
Copy link
Author

get_home_url() returns the home url without a trailing slash and if the site has a trailing slash it will result in the service worker having a wrong scope.

@westonruter
Copy link
Collaborator

Are you working with a subdirectory install? What are the specific URLs you are working with?

@nuvoPoint
Copy link
Author

Yes Sir, it's a multi site installation with subdirectory setup, where each language/country has urls like https://www.site.com/en/ and https://www.site.com/da/

@westonruter
Copy link
Collaborator

The intended behavior is for each site on a multisite install to get its own service worker installed. You're saying that if you go to the English site then the scope is wrong. Is it /en? But then why is your change for the start_url and not the scope?

@nuvoPoint
Copy link
Author

If I do a lighthouse audit I can see the following issue:

Does not register a service worker that controls page and start_url
This page is controlled by a service worker, however the start_url ("https://localhost/en") is not in the service worker's scope ("https://localhost/en/").

There is no issues with the fix and each site will have its own service worker.

@westonruter
Copy link
Collaborator

Ok, so the issue is that the start URL needs to be trailing slashed to match the scope, right?

In that case, trailingslashit should be used to unconditionally force the slash.

@nuvoPoint
Copy link
Author

Correct, but if any other site has a setup without a trailing slash in the home_url then there might be a issue for them eg. https://www.site.com.

What user_trailingslashit ensures is that if the permalinks config is setup to have a trailing slash it keeps it otherwise it will remove it.

@westonruter
Copy link
Collaborator

But if the start_url is /en but the scope is /en/ then we need to make sure that the start URL is in the scope.

@nuvoPoint
Copy link
Author

nuvoPoint commented Jun 28, 2019

I have tried to look into the code for where the scope it getting set and it seems it in wp_print_service_workers and the home_url( '/', 'relative' ) call.

So we could force set a trailing slash for both the start_url and the scope, but wouldn't it be better to use user_trailingslashit( get_home_url() ) in both places and let the users permalinks config handle that decision?

@nuvoPoint
Copy link
Author

@westonruter Is there anything you need me to do?

@westonruter
Copy link
Collaborator

@nuvoPoint Please share specifically how you have configured your site so I can reproduce your setup. You have multisite subdirectory install, but you have turned off trailing slashes?

@westonruter
Copy link
Collaborator

Please share specifically how you have configured your site so I can reproduce your setup. You have multisite subdirectory install, but you have turned off trailing slashes?

@nuvoPoint ☝️

@nuvoPoint
Copy link
Author

nuvoPoint commented Jul 30, 2019

Sorry, I did not see your last comment.

On my localhost it's setup on NginX with nothing more than:

server {
    listen 80 default_server;
    listen 443 ssl default_server;
    server_name _ ;
    root "C:/laragon/www/";
    
    index index.html index.htm index.php;
 
    # Access Restrictions
    allow       127.0.0.1;
    deny        all;
    # allow 192.168.0.0/24;
 
    include "C:/laragon/etc/nginx/alias/*.conf";

    location / {
        #try_files $uri $uri/ =404;
		try_files $uri $uri/ /index.php?$args;
		autoindex on;
    }
    
    location ~ \.php$ {
        include snippets/fastcgi-php.conf;
        fastcgi_pass php_upstream;		
        #fastcgi_pass unix:/run/php/php7.0-fpm.sock;
    }
    
    # Enable SSL
    ssl_certificate "C:/laragon/etc/ssl/laragon.crt";
    ssl_certificate_key "C:/laragon/etc/ssl/laragon.key";
    ssl_session_timeout 5m;
    ssl_protocols TLSv1 TLSv1.1 TLSv1.2;
    ssl_ciphers ALL:!ADH:!EXPORT56:RC4+RSA:+HIGH:+MEDIUM:+LOW:+SSLv3:+EXP;
    ssl_prefer_server_ciphers on;
	
	
    charset utf-8;
	
    location = /favicon.ico { access_log off; log_not_found off; }
    location = /robots.txt  { access_log off; log_not_found off; }
    location ~ /\.ht {
        deny all;
    }
	
	# Adds support for WP Multisites, where the setup is as subfolders.
	# Must be last, as the rewrites break if statements if placed before.
	if (!-e $request_filename) {
	  # Adds a trailing slash to /wp-admin
	  rewrite /wp-admin$ $scheme://$host$uri/ permanent;
	  # Rewrites so that /our-site/wp-{admin/content/includes} works.
	  rewrite ^/[_0-9a-zA-Z-]+(/wp-.*) $1 last;
	  # Same as the above, but for php files.
	  rewrite ^/[_0-9a-zA-Z-]+(/.*\.php)$ $1 last;
	}
}

And in wp-config:

define('MULTISITE', true);
define('SUBDOMAIN_INSTALL', false);
define('DOMAIN_CURRENT_SITE', 'localhost');
define('PATH_CURRENT_SITE', '/');
define('SITE_ID_CURRENT_SITE', 1);
define('BLOG_ID_CURRENT_SITE', 1);

$_SERVER['HTTPS'] = 'on';
if ( ! defined( 'WP_CLI' ) ) {
	define( 'COOKIE_DOMAIN', $_SERVER['HTTP_HOST'] );
}
define( 'FORCE_SSL_ADMIN', true );

Our live servers have a bit different setup on NginX, but with the same results.

Please let me know if your need anything else?

@westonruter westonruter added this to the 0.3 milestone Jul 31, 2019
@westonruter
Copy link
Collaborator

westonruter commented Jul 31, 2019

@nuvoPoint See my comment about suggesting home_url( '/' ) to follow the pattern of how core does it. This will match the scope which is done via home_url( '/', 'relative' ). So both are trailing-slashed by default. It seems the front page in WordPress always will get trailingslashed, unless extraordinary efforts are done to remove the slash.

There is an open issue in the service worker spec regarding scope resolution for URLs that don't end in slashes: w3c/ServiceWorker#1272

See also this related issue I just opened: #203

@nuvoPoint
Copy link
Author

Forcing the start_url to have a slash with home_url( '/' ) will work in our setup and fully acceptable for me. But it will might not work for people who has made "extraordinary efforts" to remove the slash, which user_trailingslashit( get_home_url() ) should take care of.

@westonruter
Copy link
Collaborator

I think this is the best option until w3c/ServiceWorker#1272 is resolved.

@nuvoPoint
Copy link
Author

That's fine for us - thanks!
Do you need us to do anything or are you just updating it?

@westonruter
Copy link
Collaborator

@nuvoPoint I just updated it. Can you confirm the change works now for you?

@nuvoPoint
Copy link
Author

I can confirm that it works!

@westonruter westonruter merged commit 2cbf974 into GoogleChromeLabs:master Aug 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants