nystudio107 / craft-seomatic

SEOmatic facilitates modern SEO best practices & implementation for Craft CMS 3. It is a turnkey SEO system that is comprehensive, powerful, and flexible.
https://nystudio107.com/plugins/seomatic
Other
165 stars 69 forks source link

Overwritting rel=alternate in templates reverts back to default after some time #189

Closed XhmikosR closed 6 years ago

XhmikosR commented 6 years ago

So this is a weird issue :/

We are using Craft Pro v3.0.18 with craft-seomatic v3.1.6.

In our templates we do this:

{% hook 'seo' %}
{% do seomatic.tag.get('referrer').include(false) %}
{% do seomatic.link.get('alternate').hreflang('en-US') %}

This works fine for like ~ a day. After that, the link alternate reverts back to x-default. If I clear Craft's caches, then it goes back to en-US.

I notice this issue on our production app but not in our staging app.

We override seomatic's config like this (not sure if it's 100% right)

return [
    // The server environment, either `live`, `staging`, or `local`
    'environment' => getenv('ENVIRONMENT') === 'production' ? 'live' : getenv('ENVIRONMENT')
];

I know the info might not be sufficient but I'm not sure what else info I can provide (at least publicly).

khalwat commented 6 years ago

errr... there's no {% hook 'seo' %} in SEOmatic -- I think you may be thinking about Ether's SEO plugin?

Make sure you don't have both installed...

XhmikosR commented 6 years ago

I definitely don't have that installed but I'll double check the template initialization and remove it. But the issue indeed happens.

On Thu, Aug 2, 2018, 23:04 Andrew Welch notifications@github.com wrote:

errr... there's no {% hook 'seo' %} in SEOmatic -- I think you may be thinking about Ether's SEO plugin?

Make sure you don't have both installed...

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/nystudio107/craft-seomatic/issues/189#issuecomment-410050771, or mute the thread https://github.com/notifications/unsubscribe-auth/AAVVtdLEO80vaxnzdq_sqgBShPkmx2nlks5uM1s0gaJpZM4Vsqxm .

khalwat commented 6 years ago

Well, you state that you do this in your templates:

{% hook 'seo' %}
{% do seomatic.tag.get('referrer').include(false) %}
{% do seomatic.link.get('alternate').hreflang('en-US') %}

Unless that's just a copy/paste error, either the SEO plugin is installed, or that template should be throwing an error.

As for your issue, make sure that the SEOmatic commands are not inside of a {% cache %} tag?

XhmikosR commented 6 years ago

Errors are not thrown in all environments as you already know. But anyway I removed that leftover.

Regarding cache tags, no I don't have that either.

Let me observe this a little more and come back to you next week.

khalwat commented 6 years ago

Very strange. Do let me know what you find out. Because it's something that works, but then seems to "not work" over time, my guess is there's some kind of a caching issue involved here.

XhmikosR commented 6 years ago

Ok, so this started happening on my staging environment too with 3.1.9.

I have already removed the seo leftover and just do

{% do seomatic.tag.get('referrer').include(false) %}
{% do seomatic.link.get('alternate').hreflang("#{craft.app.language}") %}

I think the issue might be with the order I'm using in the head. Like where I'm calling {{ head() }}.

XhmikosR commented 6 years ago

In fact, with v3.1.9 it's always x-default, while with 3.1.8 it worked on my staging and dev environments properly, but reverted back to x-default after some time on production.

So, I think whatever you changed after v3.1.8 definitely triggers my issue :)

khalwat commented 6 years ago

Is there a reason why you're explicitly calling {{ head() }}?

khalwat commented 6 years ago

I just tested the code you're using here, and I'm seeing it work as expected when I add in:

{% do seomatic.tag.get('referrer').include(false) %}

I'd get away from explicitly calling {{ head() }} unless this is something you absolutely need to be doing.

XhmikosR commented 6 years ago

Thanks for the suggestion.

I'm gonna try without explicitly calling head and see how it goes for a couple of days. I'll let you know.

XhmikosR commented 6 years ago

Ah, I remember why I resorted to calling head. I wanted some of the stuff I output in head to be at the end of it, because the seomatic stuff are currently output there. Is there any way I can overwrite it?

khalwat commented 6 years ago

You can use {% js at POS_HEAD %} to add things to the head, but it probably won't be at the exact end. Does it matter?

XhmikosR commented 6 years ago

Not to me a lot, but marketing... :P

Currently, I worked around it by not using view.registerCssFile/registerJsFile so that the CSS files are to the top and GTM to the end.

But, if I had an option to load the GTM script even on staging (maybe a dropdown to override the default setting), I could get rid of my custom GTM script. And, yeah, we need GTM in staging too because we load a bunch of other stuff via GTM like cookiebot etc.

PS. I'm gonna deploy today/tomorrow tops, so I should have news in 1-2 days if the x-default issue still persists with 3.1.11.

khalwat commented 6 years ago

I don't understand why marketing cares where in the <head> something goes?

khalwat commented 6 years ago

You do have the option to load GTM even on the staging site. In Twig, just do something like this:

{% do seomatic.script.get('googleTagManager').include(true) %}

This will force it to include the script regardless of the environment. You can also adjust the dataLayer:

{% do seomatic.script.get('googleTagManager').dataLayer({
    'woof': 'bark'
}) %}

So you can pass in different things depending on the environment. Documented here: https://github.com/nystudio107/craft-seomatic#google-tag-manager

khalwat commented 6 years ago

I'm going to assume the above answers this issue.

XhmikosR commented 6 years ago

Please don't hurry. I'm still waiting to see if the issue happens.

khalwat commented 6 years ago

Did you see my comment above about how you can have GTM working on staging? And then should be able to avoid having to use {{ head() }}?

XhmikosR commented 6 years ago

Yes, I did, and I have already made a few changes. But it still seemed to happen even without head(), so I need to verify.

XhmikosR commented 6 years ago

@khalwat: this definitely happens in my case. I have to log into the production app and clear Craft's caches.

khalwat commented 6 years ago

Clearing which cache specifically changes the result?

Next time you see it happen, clear them one at a time until it "fixes" itself, it'll be helpful in tracking down what is doing on.

XhmikosR commented 6 years ago

I will, but we definitely need this fixed ASAP, because it affects our SEO (and I got marketing all over me :P)

khalwat commented 6 years ago

I'm not convinced it's an outright bug; it's more likely to be something specific to your setup and/or templates.

I use code very much like this on a number of live sites, without the issues you've mentioned here.

XhmikosR commented 6 years ago

Yeah, but regardless, it happens with SEOmatic ;)

XhmikosR commented 6 years ago

@khalwat: please reopen this since it's an ongoing issue. The fact that you can't reproduce (yet) doesn't mean it's not an issue.

I had the chance to pinpoint this to the SEOmatic frontend template caches clear cache option. After that, it respects my changes.

My current SEOmatic related template changes are these:

{% do seomatic.link.get('alternate').hreflang("#{craft.app.language}") %}
{% do seomatic.script.get('googleTagManager').include(true) %}
{% do seomatic.tag.get('referrer').include(false) %}
XhmikosR commented 6 years ago

Oh, also, my current version is SEOmatic 3.1.13.1 and Craft 3.0.19

khalwat commented 6 years ago

Right, so then the question is... why would it work (the cache being correct) and then suddenly stop working?

XhmikosR commented 6 years ago

@khalwat: that is something I can't figure out. I know it works after I clear the cache, and then breaks like ~24 hours after that without us making any changes to the production site. The staging app does not have this issue ever. I've also made sure there are no template errors on my side, which when I opened this issue, that wasn't the case.

I'm not sure what more info I can give you. My SEOmatic template changes are above.

khalwat commented 6 years ago

So if the staging server never has this issue, I'd start to look for differences in Craft & server config between the staging and production servers.

XhmikosR commented 6 years ago

Already did and there is none, apart from the environment variable set to production for the production.

khalwat commented 6 years ago

Have you changed things so that you're no longer calling {{ head() }} and instead included GTM as per the above code?

If this is working fine on the staging server, there has to be some difference with the production server that's bring it out.

khalwat commented 6 years ago

Added a config option so you can just disable it there: https://github.com/nystudio107/craft-seomatic/commit/f75350cc8f00b0c5d76dce342bcfe8afad9fa8d5

XhmikosR commented 6 years ago

Thanks, but this is not the solution to my issue. Because the problem is that rel alternate along with other frontend entries are reverted to random value. It's like every ~24 hours SEOmatic believes we are not in production or something, because it drops any custom template overrides, including rel alternate, meta robots is set to none even though the env is production.

khalwat commented 6 years ago

If you believe the issue is environmental, it should be pretty easy to test. Create some kind of a tag in SEOmatic (it can be whatever you want), something like:

{% do seomatic.tag.create({
    'content': seomatic.config.environment,
    'name': 'seomaticEnvironment',
}) %}

{% do seomatic.tag.create({
    'content': getenv('ENVIRONMENT'),
    'name': 'craftEnvironment',
}) %}

And then you will actually have meta tags (which will be ignored by browsers/bots) that will tell you explicitly what environment is running.

Although I'm open to the idea that this is an SEOmatic bug, from everything that you've stated—and from using SEOmatic extensively in the field—I'm leaning towards some kind of environmental/server setup issue.

So questions that need answering are:

  1. Are the staging and production servers running on the same shared VPS/instance?

  2. What caching method is being used for Yii2 on this server?

  3. I'd also consider a slightly more straightforward seomatic.php, such as:

return [

    // All environments
    '*' => [
        // The public-facing name of the plugin
        'pluginName' => 'SEOmatic',

        // Should SEOmatic render metadata?
        'renderEnabled' => true,

        // The server environment, either `live`, `staging`, or `local`
        'environment' => 'live',

        // Should SEOmatic display the SEO Preview sidebar?
        'displayPreviewSidebar' => true,

        // Should SEOmatic display the SEO Analysis sidebar?
        'displayAnalysisSidebar' => true,

        // If `devMode` is on, prefix the <title> with this string
        'devModeTitlePrefix' => '🚧 ',

        // The separator character to use for the `<title>` tag
        'separatorChar' => '|',

        // The max number of characters in the `<title>` tag
        'maxTitleLength' => 70,

        // The max number of characters in the `<meta name="description">` tag
        'maxDescriptionLength' => 320,
    ],

    // Live (production) environment
    'production'  => [
        // The server environment, either `live`, `staging`, or `local`
        'environment' => 'live',
    ],

    // Staging (pre-production) environment
    'staging'  => [
        // The server environment, either `live`, `staging`, or `local`
        'environment' => 'staging',
    ],

    // Local (development) environment
    'local'  => [
        // The server environment, either `live`, `staging`, or `local`
        'environment' => 'local',
    ],
];

...as shown in the documention: https://github.com/nystudio107/craft-seomatic#plugin-settings

XhmikosR commented 6 years ago

Thanks for the reply, @khalwat. Please keep the issue open for a few days until I try your suggestions and have more info.

EDIT:

This doesn't seem to work @khalwat

    {% do seomatic.tag.create({
        "content" => seomatic.config.environment,
        "name" => "seomaticEnvironment"
    }) %}

    {% do seomatic.tag.create({
        "content" => getenv("ENVIRONMENT"),
        "name" => "craftEnvironment"
    }) %}

A hash key must be followed by a colon (:). Unexpected token "operator" of value "=" ("punctuation" expected with value ":").

EDIT2:

Nvm, I changed => to :.

XhmikosR commented 6 years ago

About my server setup.

It's 2 fortrabbit apps, with the only difference in the ENVIRONMENT variable I set. Everything else is the same.

XhmikosR commented 6 years ago

@khalwat: so I just pushed this on my production app, using this config for SEOmatic + the meta tags

return [
    // Live (production) environment
    'production'  => [
        // The server environment, either `live`, `staging`, or `local`
        'environment' => 'live'
    ],

    // Staging (pre-production) environment
    'staging'  => [
        // The server environment, either `live`, `staging`, or `local`
        'environment' => 'staging'
    ],

    // Local (development) environment
    'dev'  => [
        // The server environment, either `live`, `staging`, or `local`
        'environment' => 'local'
    ]
];
<meta name="seomaticEnvironment" content="staging">
<meta name="craftEnvironment" content="production">
XhmikosR commented 6 years ago

On my staging app, both vars are staging so something seems wrong somewhere. Any ideas?

khalwat commented 6 years ago

Please also put a default in there as per my example:

    '*' => [
        // The public-facing name of the plugin
        'pluginName' => 'SEOmatic',

        // Should SEOmatic render metadata?
        'renderEnabled' => true,

        // The server environment, either `live`, `staging`, or `local`
        'environment' => 'live',

        // Should SEOmatic display the SEO Preview sidebar?
        'displayPreviewSidebar' => true,

        // Should SEOmatic display the SEO Analysis sidebar?
        'displayAnalysisSidebar' => true,

        // If `devMode` is on, prefix the <title> with this string
        'devModeTitlePrefix' => '🚧 ',

        // The separator character to use for the `<title>` tag
        'separatorChar' => '|',

        // The max number of characters in the `<title>` tag
        'maxTitleLength' => 70,

        // The max number of characters in the `<meta name="description">` tag
        'maxDescriptionLength' => 320,
    ],

We want to provide sane defaults in case for whatever reason it doesn't match any environment.

XhmikosR commented 6 years ago

Well, with those in place it's correct

<meta name="seomaticEnvironment" content="live">
<meta name="craftEnvironment" content="production">
XhmikosR commented 6 years ago

But it's random, and I don't think it has to do with the all environment settings being there.

I tried commenting out one by one the all env settings, but I can't conclude which one triggers this. It does get triggered randomly after I clear Craft's cache (all) regardless of the setting I comment out.

khalwat commented 6 years ago

Well now that's interesting. What that means is you're having requests come in without the ENVIRONMENT set properly for some reason.

This likely means that for other things, this is set improperly as well. How are you setting ENVIRONMENT?

XhmikosR commented 6 years ago

Via fortrabbit's environment variables. This is the same as the staging app.

khalwat commented 6 years ago

Here's the relevant code from vendor/craftcms/cms/src/bootstrap/bootstrap.php:

// Set the environment
$environment = $findConfig('CRAFT_ENVIRONMENT', 'env') ?: ($_SERVER['SERVER_NAME'] ?? null);

...and then here is what (should) be in your web/index.php:

// Load and run Craft
define('CRAFT_ENVIRONMENT', getenv('ENVIRONMENT') ?: 'production');

So what I think is happening is that for whatever reason, on some requests, ENVIRONMENT is not set or is not set to what you're expecting it to be, so CRAFT_ENVIRONMENT then defaults to production which is what is causing this to "just work" now that you've added a wildcard.

khalwat commented 6 years ago

FYI, this is where Craft merges in the plugin's settings with the contents of the config file:

https://github.com/craftcms/cms/blob/develop/src/services/Plugins.php#L776

...which ends up calling this:

https://github.com/craftcms/cms/blob/develop/src/services/Config.php#L203

XhmikosR commented 6 years ago

Thanks for the replies,I really appreciate it. Now i will double check web index file tomorrow but iirc its correct.

The craft meta though always sticks to production. Do you have any idea how we can debug this further?

On Mon, Aug 13, 2018, 21:30 Andrew Welch notifications@github.com wrote:

FYI, this is where Craft merges in the plugin's settings with the contents of the config file:

https://github.com/craftcms/cms/blob/develop/src/services/Plugins.php#L776

...which ends up calling this:

https://github.com/craftcms/cms/blob/develop/src/services/Config.php#L203

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/nystudio107/craft-seomatic/issues/189#issuecomment-412618459, or mute the thread https://github.com/notifications/unsubscribe-auth/AAVVtRWFKCsXitMvaYU75iBSNlQSdKhZks5uQcXNgaJpZM4Vsqxm .

khalwat commented 6 years ago

SEOmatic isn't doing anything special here; it's just leveraging the built-in multi-environment config file support.

You might want to ask support at FortRabbit as well, maybe @o_stark will have some insight.

XhmikosR commented 6 years ago

My web/index.php is fine too.

The environment meta tag is always fine; the SEOmatic meta tag reverts back randomly. I will ask fortrabbit support too, but this is a pretty difficult issue to debug for sure.

XhmikosR commented 6 years ago

After looking at the source code, I still think this is a bug in SEOmatic somewhere around here https://github.com/nystudio107/craft-seomatic/blob/9a0af099e84271b94424c17f87f17b3987b3018a/src/helpers/Config.php#L58-L74

XhmikosR commented 6 years ago

After discussing a lot with @ostark, here is what I know for sure: unless I explicitly set the '*' environment options, SEOmatic wrongfully sets its environment to staging.

So this triggers the bug for sure:

return [
    // Live (production) environment
    'production'  => [
        // The server environment, either `live`, `staging`, or `local`
        'environment' => 'live'
    ],

    // Staging (pre-production) environment
    'staging'  => [
        // The server environment, either `live`, `staging`, or `local`
        'environment' => 'staging'
    ],

    // Local (development) environment
    'dev'  => [
        // The server environment, either `live`, `staging`, or `local`
        'environment' => 'local'
    ]
];

2production

If I use your suggestion and include the all environment settings, then the issue seems gone. Seems, because I need to confirm after 24 hours. So this seems to work:

return [

    // All environments
    '*' => [
        // The public-facing name of the plugin
        'pluginName' => 'SEOmatic',

        // Should SEOmatic render metadata?
        'renderEnabled' => true,

        // The server environment, either `live`, `staging`, or `local`
        'environment' => 'live',

        // Should SEOmatic display the SEO Preview sidebar?
        'displayPreviewSidebar' => true,

        // Should SEOmatic display the SEO Analysis sidebar?
        'displayAnalysisSidebar' => true,

        // If `devMode` is on, prefix the <title> with this string
        'devModeTitlePrefix' => '&#x1f6a7; ',

        // The separator character to use for the `<title>` tag
        'separatorChar' => '|',

        // The max number of characters in the `<title>` tag
        'maxTitleLength' => 70,

        // The max number of characters in the `<meta name="description">` tag
        'maxDescriptionLength' => 320
    ],

    // Live (production) environment
    'production'  => [
        // The server environment, either `live`, `staging`, or `local`
        'environment' => 'live'
    ],

    // Staging (pre-production) environment
    'staging'  => [
        // The server environment, either `live`, `staging`, or `local`
        'environment' => 'staging'
    ],

    // Local (development) environment
    'dev'  => [
        // The server environment, either `live`, `staging`, or `local`
        'environment' => 'local'
    ]
];

1staging

So, this does still seem like a bug to my eyes.