Page MenuHomePhabricator

Two link rel="canonical" if $wgEnableCanonicalServerLink and $mfNoIndexPages is true
Closed, ResolvedPublic1 Estimated Story Points

Description

Related bug: https://phabricator.wikimedia.org/T225876

if $wgEnableCanonicalServerLink is true - MediaWiki add to each page

<link rel="canonical" href="https://example.com/wiki/Page_Name"/>

and if $mfNoIndexPages is true - it also add to mobile pages

<link rel="canonical" href="https://example.com/wiki/Page_Name"/>

looks like this is bug, if $wgEnableCanonicalServerLink is true - and link rel="canonical" is added to page, directive $mfNoIndexPages should not add second link rel="canonical".

Is it possible to fix this small cosmetic bug? Bug is here:
MobileFrontend/includes/MobileFrontend.hooks.php

// an canonical/alternate link is only useful, if the mobile and desktop URL are different
// and $wgMFNoindexPages needs to be true
if ( $mfMobileUrlTemplate && $mfNoIndexPages ) {
        $link = false;

        if ( !$context->shouldDisplayMobileView() ) {
                // add alternate link to desktop sites - bug T91183
                $desktopUrl = $title->getFullUrl();
                $link = [
                        'rel' => 'alternate',
                        'media' => 'only screen and (max-width: ' . self::DEVICE_WIDTH_TABLET . ')',
                        'href' => $context->getMobileUrl( $desktopUrl ),
                ];
        } elseif ( !$title->isSpecial( 'MobileCite' ) ) {
                // Add canonical link to mobile pages (except for Special:MobileCite),
                // instead of noindex - bug T91183.
                $link = [
                        'rel' => 'canonical',
                        'href' => $title->getFullUrl(),
                ];
        }

        if ( $link !== false ) {
                $out->addLink( $link );
        }
}

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 16 2019, 7:21 AM
pmiazga claimed this task.Jun 18 2019, 3:12 PM
ovasileva triaged this task as Medium priority.Jun 24 2019, 9:31 PM
pmiazga added a subscriber: phuedx.EditedJun 26 2019, 7:43 PM

I can reproduce this, it's an issue.

@DIKW_Pyramid Can I repurpose the T225876: MobileFrontend configuration directive $mfNoIndexPages - wrong description and wrong name to:

  • remove the old MFNoIndexPages config variable
  • add new config MFInjectAlternateLinkToDesktopPages that adds only alternate links to desktop pages
  • remove the code that adds canonical URL from MobileFrontend

And then close this task as duplicate?

Currently the MFNoIndexPages adds

  • canonical URL if user is viewing mobile site
  • alternate url if user is viewing desktop site

I don't think it's useful to have same feature (add canonical URL) in two places, if MediaWiki Core supports it, then MobileFrontend should depend upon MediaWiki handling, not create it's own.
Also, the MediaWiki one (wgEnableCanonicalServerLink) is set to true, so mobile pages gets the canonical URL anyway.

/cc @phuedx

@pmiazga please see https://developers.google.com/search/mobile-sites/mobile-seo/separate-urls
before doing any changes.

link rel="canonical" on mobile page is required, or search engines will think what this is two different sites.

link rel="alternate" on desktop pages is useless without link rel="canonical" on mobile pages.

So, if link rel="alternate" added to desktop pages - in same time link rel="canonical" on mobile pages MUST be added or site indexing by search engines will be broken and search engines will think what it is two different sites instead of only one site with desktop and mobile versions.

If add new directive $MFInjectAlternateLinkToDesktopPages - it will be too hard for MediaWiki users - they should know all details how search engines index mobile and desktop sites and they should manually configure $wgEnableCanonicalServerLink to true or wiki indexing with mobile and desktop versions will be broken.

By default $wgEnableCanonicalServerLink is false, and by default all wiki with mobile versions will be broken.
I think this is not good. Wiki indexing should work correctly out of the box without additional tuning.

$MFInjectAlternateLinkToDesktopPages without $wgEnableCanonicalServerLink is dangerous and useless.

I propose to just add one more check in MobileFrontend/includes/MobileFrontend.hooks.php:

If $wgEnableCanonicalServerLink is true - not add second link rel="canonical" on mobile page:

// an canonical/alternate link is only useful, if the mobile and desktop URL are different
// and $wgMFNoindexPages needs to be true
if ( $mfMobileUrlTemplate && $mfNoIndexPages ) {
        $link = false;

        if ( !$context->shouldDisplayMobileView() ) {
                // add alternate link to desktop sites - bug T91183
                $desktopUrl = $title->getFullUrl();
                $link = [
                        'rel' => 'alternate',
                        'media' => 'only screen and (max-width: ' . self::DEVICE_WIDTH_TABLET . ')',
                        'href' => $context->getMobileUrl( $desktopUrl ),
                ];
        } elseif ( !$title->isSpecial( 'MobileCite' ) ) {
                // Add canonical link to mobile pages (except for Special:MobileCite),
                // instead of noindex - bug T91183.
                if ($wgEnableCanonicalServerLink !== true) {
                        $link = [
                                'rel' => 'canonical',
                                'href' => $title->getFullUrl(),
                        ];
                }
        }

        if ( $link !== false ) {
                $out->addLink( $link );
        }
}

and nothing else on this task.

All other details about name and description of $wgMFNoindexPages directive - I propose discuss In T225876.

@DIKW_Pyramid makes sense, sorry I should be more explicit, by

By default $wgEnableCanonicalServerLink is true,

I meant the production config https://github.com/wikimedia/operations-mediawiki-config/blob/46daeb8443ffa6fafddbbae0b8b79d056499e167/wmf-config/InitialiseSettings.php#L2145
The canonical URL will be always added, even for the MobileCite page (which supposedly shouldn't have that canonical url).

I don't see a valid reason two configs that do the same thing. Also, MobileFrontend as MediaWiki extension should respect MediaWiki configs/permissions. If MediaWiki Core says "no" to something, MF shouldn't enforce that.
What I can propose in this situation is to:

  • still keep MFInjectAlternateLinkToDesktopPages - maybe use a better name, the one I proposed was my first guess
  • if MFInjectAlternateLinkToDesktopPages is set to true, but the wgEnableCanonicalServerLink is set to false then log an error, that injecting alternate links to desktop is no good without canonical urls

I don't want MF to reinvent the wheel, if MediaWiki supports something like adding canonical urls, we should use the MediaWiki logic first, then implement our own workarounds. IMHO it's bad when core disables something, but the extension
re-enables it via different config.

@pmiazga, by default $wgEnableCanonicalServerLink is false because it is no reliable way to detect $wgServer and $wgCanonicalServer. As I understand - this is reason why $wgEnableCanonicalServerLink is not true by default in MediaWiki.

Directive $wgMFNoindexPages ($wgMFEnableRelationship) is true by default because if wiki have mobile and desktop version - search engines should always understand relationship between desktop and mobile versions (see T91183 for details).

Breaking of relationship between mobile and desktop versions is useless and only bringing harm and probably nobody want to break this relationship. May be switch $wgMFNoindexPages ($wgMFEnableRelationship) should be removed at all and it should be considered always true?

It is no any reason now to set $wgMFNoindexPages ($wgMFEnableRelationship) to false after T91183.

For correct working of directive $wgMFEnableRelationship it is required to set link rel="alternate" on desktop pages and link rel="canonical" on mobile pages.

This is Google and other search engines requirement: https://developers.google.com/search/mobile-sites/mobile-seo/separate-urls It is not reinventing the wheel by MF.

link rel="alternate" on desktop pages without link rel="canonical" on mobile pages is incomplete.

core is not disables $wgEnableCanonicalServerLink, core just has no way to reliable enable $wgEnableCanonicalServerLink on any MediaWiki installation.

Probably now you see this situation only from Wikipedia point of view? But in the world there are many other sites build on MediaWiki technology, and all these sites with MF are now work fine with default settings without any additional tuning. Probably we should not break all these sites and we should consider backward compatibility?

Probably we should not make situation worse than it is now?

Two link rel="canonical" is not a problem at all, it is just a small cosmetic bug.

Following on from @DIKW_Pyramid's comment above:

At the very least that body of code should be moved to its own function (perhaps a hook handler in a separate class?) so that it can be written, updated as suggested, and tested more cleanly, i.e.

class MobileSeoHookHandler {
    public function handleOnBeforePageDisplay( OutputPage $out ) {
    // See https://phabricator.wikimedia.org/T91183 for history.
    // See https://developers.google.com/search/mobile-sites/mobile-seo/separate-urls for context.

    $services = MediaWikiServices::getInstance();
    $mobileUrlTemplate = $services->getService( 'MobileFrontend.Context' )->getMobileUrlTemplate();

    $config = $services->getService( 'MobileFrontend.Config' );
    $noindexPages = $config->get( 'MFNoindexPages' );
    $enableCanonicalServerLink = $config->get( 'EnableCanonicalServerLink' );

    // Do the URLs for the desktop and mobile sites vary ($wgMFMobileUrlTemplate)? If not, then
    // there's no relationship to signal to search engines. If so, then only signal the
    // relationship if we're told to ($wgMFNoindexPages).
    if ( !$mobileUrlTemplate || !$noindexPages ) {{
        return;
    }

    // Mobile
    if (
        $context->shouldDisplayMobileView()

        // Special:MobileCite is a mobile-only special page.
        // TODO: What about Special:MobileMenu?
        && !$title->isSpecial( 'MobileCite' )

        // Is MediaWiki already configured to add an external resource link indicating the canonical
        // version of the site?
        && !$enableCanonicalServerLink )
    {
        $out->addLink( [
            'rel' => 'canonical',
            'href' => $title->getFullURL(),
        ] );

        return;
    }

    // Desktop
    // FIXME: Define DEVICE_WIDTH_TABLET elsewhere.
    $link = [
        'rel' => 'alternate',
        'media' => 'only screen and (max-width: ' . MobileFrontendHooks::DEVICE_WIDTH_TABLET . ')',
        'href' => $context->getMobileUrl( $title->getFullURL() ),
    ];
}
pmiazga removed pmiazga as the assignee of this task.Jul 9 2019, 4:53 PM
pmiazga moved this task from Needs Prioritization to Upcoming on the Readers-Web-Backlog board.
pmiazga added a subscriber: pmiazga.
ovasileva set the point value for this task to 1.Jul 9 2019, 4:54 PM

Change 528791 had a related patch set uploaded (by Pmiazga; owner: Pmiazga):
[mediawiki/extensions/MobileFrontend@master] When EnableCanonicalServerLink is set, don't add canonical link

https://gerrit.wikimedia.org/r/528791

pmiazga removed pmiazga as the assignee of this task.Aug 7 2019, 2:25 PM

Change 528791 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] When EnableCanonicalServerLink is set, don't add canonical link

https://gerrit.wikimedia.org/r/528791

Edtadros reassigned this task from Edtadros to Jdlrobson.Aug 8 2019, 2:09 PM
Edtadros added subscribers: Jdlrobson, Edtadros.

@Jdlrobson this needs QA steps.

Jdlrobson reassigned this task from Jdlrobson to pmiazga.Aug 8 2019, 2:15 PM
Niedzielski closed this task as Resolved.Aug 12 2019, 2:59 PM
Niedzielski added a subscriber: Niedzielski.
  • When $wgEnableCanonicalServerLink = true, <link rel="canonical" href="http://localhost:8181/wiki/Foo"> is included once on mobile pages regardless of $wgMFNoindexPages or $wgMobileUrlTemplate state.
  • When $wgEnableCanonicalServerLink = false and $wgMobileUrlTemplate is unset, the canonical link is not included on mobile pages regardless of $wgMFNoindexPages state.
  • When $wgEnableCanonicalServerLink = false and $wgMFNoindexPages = 'foo':
    • When $wgMFNoindexPages = true, the canonical link is included once on mobile pages.
    • When $wgMFNoindexPages = false, the canonical link is not included on mobile pages.