Add link rel=canonical tag for `/log-in` to tell search engine it is the same page as `/wp-login.php` #15297

Tug · 2017-06-20T13:50:55Z

Redirecting /wp-login.php to /log-in resulted in some internal checks raising errors regarding our meta and link tags.
This fix will tell search engine that our new /log-in page is essentially the same as the legacy one (/wp-login.php).

Testing instructions

Load this branch locally and boot your server (npm start)
Run curl "http://calypso.localhost:3000/log-in" | grep '<link rel="canonical"' and check that the tag is indeed in the returned html
Note that curl "http://calypso.localhost:3000/log-in?redirect_to=https%3A%2F%2Fwordpress.com%2F" | grep '<link rel="canonical"' will not work
Visit http://calypso.localhost:3000/log-in?redirect_to=https%3A%2F%2Fwordpress.com%2F (logged out), inspect the html once the js has loaded and check that the tag is present.
Note that curl "http://calypso.localhost:3000/log-in/fr" | grep '<link rel="canonical"' will not work
Visit http://calypso.localhost:3000/log-in/fr (logged out), inspect the html once the js has loaded and check that the tag is present.

Reviews

Code
Product

matticbot · 2017-06-20T13:50:56Z

Test live: https://calypso.live/?branch=add/rel-canonical

Tug · 2017-06-30T13:20:48Z

Localized SSR would require us to async load and cache the translation files with some sort of expire delay. It's not trivial so I suggest we don't enable it for this PR.

fabianapsimoes · 2017-07-03T08:57:37Z

Localized SSR would require us to async load and cache the translation files with some sort of expire delay. It's not trivial so I suggest we don't enable it for this PR.

Just make sure this is documented in a separate issue. along with a rough estimate of the work needed. We can then prioritize it along with the rest of our maintenance backlog.

fabianapsimoes · 2017-07-03T09:26:21Z

Hmm, in this branch I'm having issues logging in via 2FA. In /sms and /authenticor, whenever I enter a code I get this error: invalid_auth_type. If I click the button again, I get empty_two_step_nonce. I don't have these problems in production or in master. Am I missing something?

Tug · 2017-07-03T10:13:29Z

@fabianapsimoes Oh right, it needs a rebase
Edit: rebased.

fabianapsimoes · 2017-07-03T10:33:06Z

Product 👍

stephanethomas

I had two comments but the rest of the code looks good.

stephanethomas · 2017-07-03T10:59:05Z

client/login/wp-login/index.jsx

@@ -79,6 +79,9 @@ export class Login extends React.Component {

 					<DocumentHead title={ translate( 'Log In', { textOnly: true } ) } />

+					<DocumentHead meta={ [ { name: 'robots', content: 'noindex, follow' } ] } />


Should we really add such meta attribute here? As far I can see, we shouldn't combine a canonical tag with such attribute as it might prevent the canonical page from showing up in the search engine results.

If we decide to keep it, the meta should be merged into the DocumentHead element above.

stephanethomas · 2017-07-03T11:18:17Z

client/login/wp-login/index.jsx

@@ -79,6 +79,9 @@ export class Login extends React.Component {

 					<DocumentHead title={ translate( 'Log In', { textOnly: true } ) } />

+					<DocumentHead meta={ [ { name: 'robots', content: 'noindex, follow' } ] } />
+					<DocumentHead link={ [ { rel: 'canonical', href: 'https://wordpress.com/wp-login.php' } ] } />


What do you think of using our login() path function here instead of hardcoding the url?

Should both be merged into the DocumentHead above.

ockham · 2017-07-03T14:45:09Z

server/pages/index.js

@@ -120,13 +120,9 @@ function getCurrentCommitShortChecksum() {
 }

 function getDefaultContext( request ) {
-	let initialServerState = {};
-	// We don't cache routes with query params
-	if ( isEmpty( request.query ) ) {


Starting to cache if there are query params is a rather big change that should be highlighted in the PR desc, and deserves some discussion, since it means that cache slots will fill up way faster.

Right, I didn't highlight this enough in the description (will do) but that's one of the reason you're marked as a reviewer here, since you have experience on SSR :)
As far as I could test, I tried every isomorphic sections and didn't see any rendering issues on the current version of calypso, I might have missed some cases though.

The issue is that we need to somehow predict how this will scale under load...

@Tug do you have a defined set of query args and values we could whitelist?

The idea here is to actually ignore query params for SSR completely.

ockham · 2017-07-03T14:54:05Z

server/render/index.js

@@ -86,8 +86,7 @@ export function serverRender( req, res ) {
 		config.isEnabled( 'server-side-rendering' ) &&
 		context.layout &&
 		! context.user &&
-		isEmpty( context.query ) &&


Again, dropping this criterion might have quite an impact since we might end up calling renderToString (and thus, blocking node) way more often.

cc @ehg

Yes, this worries me. Aside from crawlers slowing the loop down, we could conceivably attack the server to DoS it with ease.

@alexsanford this where an event loop timing log could come in useful :)

I don't think this change implies calling renderToString more often. Here we're basically saying that we should ignore query parameters and just cache the first version of the page rendered. Follow up requests with the same pathname but different query parameters will end up loading that first version.
I agree it's quite a change but I tested the few logged out routes we have and I didn't see any issue with it so far.

gziolo · 2017-07-05T09:59:51Z

Can you update Jed template instead? I'm on parental leave after sleepless nights so you can ignore me :D

Tug · 2017-07-05T10:06:04Z

Can you update Jed template instead?

Yes that would be a solution. Imo, it's cleaner to have it here. But in case we can't agree on this SSR change we'll have to go with editing the server template.

I'm on parental leave after sleepless nights so you can ignore me :D

Happy parental leave Grzeg ;)

Tug · 2017-07-05T10:42:46Z

This would be a problem at least for Magic Login which is rendered server side and uses query params.
We need to explore a different solution

Tug · 2017-07-05T11:11:50Z

Let's worry about SSR in another PR and merge this change first.

…e page than /wp-login.php

scruffian · 2017-07-05T11:35:20Z

👍 for a simple solution for now. 🚢

Tug · 2017-07-05T12:38:31Z

@scruffian I added a commit to update the head tags from the client once the js has loaded. If you feel this is unnecessarily complex I'll remove it and we'll merge without it

scruffian · 2017-07-05T14:49:54Z

Nice changes @Tug. 🚢

Tug added Login [Status] In Progress labels Jun 20, 2017

Tug self-assigned this Jun 20, 2017

matticbot added the [Size] M Medium sized issue label Jun 20, 2017

Tug force-pushed the add/rel-canonical branch from 900ad24 to e0b0e1c Compare June 20, 2017 13:51

matticbot added [Size] S Small sized issue and removed [Size] M Medium sized issue labels Jun 20, 2017

Tug force-pushed the add/rel-canonical branch 2 times, most recently from 3613181 to 3244849 Compare June 21, 2017 14:55

Tug requested review from ockham, stephanethomas, gziolo and scruffian June 30, 2017 12:47

Tug added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Jun 30, 2017

Tug force-pushed the add/rel-canonical branch from d8446a8 to b9c116d Compare June 30, 2017 13:18

fabianapsimoes added [Status] Needs Author Reply and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jul 3, 2017

Tug force-pushed the add/rel-canonical branch from 63bc2c5 to 39f0333 Compare July 3, 2017 10:13

Tug added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Needs Author Reply labels Jul 3, 2017

stephanethomas requested changes Jul 3, 2017

View reviewed changes

ockham reviewed Jul 3, 2017

View reviewed changes

Tug closed this Jul 5, 2017

Tug deleted the add/rel-canonical branch July 5, 2017 10:42

matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jul 5, 2017

Tug restored the add/rel-canonical branch July 5, 2017 11:11

Tug reopened this Jul 5, 2017

Tug added 3 commits July 5, 2017 13:14

Add link rel=canonical tag to tell search engine that this is the sam…

49fc5db

…e page than /wp-login.php

Fix schema for link head tags

ca567cc

Localize the canonical url whenever it's possible

defe7ee

Tug force-pushed the add/rel-canonical branch from 8429686 to defe7ee Compare July 5, 2017 11:14

scruffian added the [Status] Ready to Merge label Jul 5, 2017

matticbot added [Size] M Medium sized issue and removed [Size] S Small sized issue labels Jul 5, 2017

Tug added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Ready to Merge labels Jul 5, 2017

Refresh head tags when the client loads

a318096

Tug force-pushed the add/rel-canonical branch from 6c67c7a to a318096 Compare July 5, 2017 12:47

scruffian added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jul 5, 2017

Handle case where attribute value is not a string

50b018e

Tug merged commit 080e14a into master Jul 5, 2017

Tug deleted the add/rel-canonical branch July 5, 2017 16:33

matticbot removed the [Status] Ready to Merge label Jul 5, 2017

ockham mentioned this pull request May 22, 2018

DocumentHead: Remove refreshHeadTags() method #25003

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add link rel=canonical tag for `/log-in` to tell search engine it is the same page as `/wp-login.php` #15297

Add link rel=canonical tag for `/log-in` to tell search engine it is the same page as `/wp-login.php` #15297

Tug commented Jun 20, 2017 •

edited by scruffian

Loading

matticbot commented Jun 20, 2017

Tug commented Jun 30, 2017

fabianapsimoes commented Jul 3, 2017 •

edited

Loading

fabianapsimoes commented Jul 3, 2017

Tug commented Jul 3, 2017 •

edited

Loading

fabianapsimoes commented Jul 3, 2017

stephanethomas left a comment

stephanethomas Jul 3, 2017

ockham Jul 3, 2017

stephanethomas Jul 3, 2017

ockham Jul 3, 2017

ockham Jul 3, 2017

Tug Jul 3, 2017

ockham Jul 3, 2017

ehg Jul 3, 2017

Tug Jul 5, 2017

ockham Jul 3, 2017

ehg Jul 3, 2017

ehg Jul 3, 2017

Tug Jul 5, 2017

gziolo commented Jul 5, 2017

Tug commented Jul 5, 2017 •

edited

Loading

Tug commented Jul 5, 2017

Tug commented Jul 5, 2017

scruffian commented Jul 5, 2017

Tug commented Jul 5, 2017

scruffian commented Jul 5, 2017

		@@ -79,6 +79,9 @@ export class Login extends React.Component {

		<DocumentHead title={ translate( 'Log In', { textOnly: true } ) } />

		<DocumentHead meta={ [ { name: 'robots', content: 'noindex, follow' } ] } />

Add link rel=canonical tag for /log-in to tell search engine it is the same page as /wp-login.php #15297

Add link rel=canonical tag for /log-in to tell search engine it is the same page as /wp-login.php #15297

Conversation

Tug commented Jun 20, 2017 • edited by scruffian Loading

Testing instructions

Reviews

matticbot commented Jun 20, 2017

Tug commented Jun 30, 2017

fabianapsimoes commented Jul 3, 2017 • edited Loading

fabianapsimoes commented Jul 3, 2017

Tug commented Jul 3, 2017 • edited Loading

fabianapsimoes commented Jul 3, 2017

stephanethomas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo commented Jul 5, 2017

Tug commented Jul 5, 2017 • edited Loading

Tug commented Jul 5, 2017

Tug commented Jul 5, 2017

scruffian commented Jul 5, 2017

Tug commented Jul 5, 2017

scruffian commented Jul 5, 2017

Add link rel=canonical tag for `/log-in` to tell search engine it is the same page as `/wp-login.php` #15297

Add link rel=canonical tag for `/log-in` to tell search engine it is the same page as `/wp-login.php` #15297

Tug commented Jun 20, 2017 •

edited by scruffian

Loading

fabianapsimoes commented Jul 3, 2017 •

edited

Loading

Tug commented Jul 3, 2017 •

edited

Loading

Tug commented Jul 5, 2017 •

edited

Loading