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

Queue appears to create a job to generate sitemap when setting is disabled #1212

Closed
strandofgenius opened this issue Sep 23, 2022 · 17 comments
Closed
Labels

Comments

@strandofgenius
Copy link

strandofgenius commented Sep 23, 2022

Describe the bug

Queue appears to be running a process to generate a sitemap when an entry type has public URLs but the sitemap setting is turned disabled in the SEOmatic content SEO settings.

Screen Shot 2022-09-22 at 3 47 34 PM

The sitemap does not get added to sitemap.xml but it is going through a process to generate it.

To reproduce

Steps to reproduce the behaviour:

  1. Create an entry type with a URI format and template
  2. Go to SEOmatic > Content SEO > Entry Type > Sitemap settings
  3. Disable all toggles related to the sitemap
  4. Entry is created via Freeform Element Connection Event
  5. Queue appears to create a job to generate a sitemap for the entry type

Expected behaviour

It is expected that because the entry type sitemap setting is disabled it would not create a queue job to generate a sitemap.

Screenshots

Screen Shot 2022-09-23 at 9 44 08 AM

Versions

  • Plugin version: 3.4.38
  • Craft version: 3.7.53.1
@khalwat
Copy link
Collaborator

khalwat commented Sep 29, 2022

So I checked on this, it it does indeed check to ensure that sitemaps are enable before generating a sitemap:

https://github.com/nystudio107/craft-seomatic/blob/develop/src/models/SitemapTemplate.php#L132

        $sitemapUrls = $metaBundle->metaSitemapVars->sitemapUrls;
        if (Seomatic::$plugin->sitemaps->anyEntryTypeHasSitemapUrls($metaBundle)) {
            $robotsEnabled = true;
            $sitemapUrls = true;
        }

...but then it also checks to see if any of the Entry Types for that section have it enabled. Possible you have multiple entry types, but only disabled it for one of them?

@khalwat khalwat closed this as completed Sep 29, 2022
@strandofgenius
Copy link
Author

There are other sub entry types but each of those also have the toggles turned off.

Screen Shot 2022-09-30 at 10 55 47 AM

Screen Shot 2022-09-30 at 10 55 40 AM

Screen Shot 2022-09-30 at 10 55 52 AM

@khalwat
Copy link
Collaborator

khalwat commented Sep 30, 2022

I'm unable to reproduce your issue here. If you have it reproducible in a local dev environment, we can try debugging it via screen share if you like?

@strandofgenius
Copy link
Author

I was able to reproduce the issue locally. It doesn't appear to happen when an entry is created from the CP, only when a new entry is created by a lead filling out a form on the front end of the site.

@strandofgenius
Copy link
Author

@khalwat any new thoughts on this? We are still seeing a pileup of queue jobs. Do you suppose this has something more to do with how the Element Connection works from the Solspace Freeform plugin works?

@khalwat
Copy link
Collaborator

khalwat commented Nov 18, 2022

This is probably due to the way that Solspace Freeform is saving the changes to the element.

@gustavs-gutmanis
Copy link

Hey @khalwat!
I ran a debugger through the code and I cannot seem to figure out what we're doing wrong.
SitemapTemplate.php#L179 seems to be reached even if $sitemapUrls = false. The cache is empty, and $immediately is set to false and then the else statement kicks in that just pushes a job.
Maybe that's something that might be causing this?
Any help would be appreciated.

@khalwat
Copy link
Collaborator

khalwat commented Dec 19, 2022

So the this method in seomatic/services/MetaBundles.php is being called after Elements::EVENT_AFTER_SAVE_ELEMENT event is triggered:

https://github.com/nystudio107/craft-seomatic/blob/develop/src/services/MetaBundles.php#L684

In looking at the code, I'm a little confused as to how it triggers anything, because the original bug says it's only for new entries coming from a frontend Solspace form, and the code checks for:

                // Is this a new source?
                if (!$isNew) {

And essentially does nothing if it's a new element. Perhaps the $event->isNew isn't set properly coming from whatever Freeform is doing to save the element?

@gustavs-gutmanis
Copy link

Thanks a lot for looking into it, I will investigate how to solve this problem on our end :)

@gustavs-gutmanis
Copy link

@khalwat
I think the issue is the element save propagation.
If there are multiple sites, the element save calls another element save for each site, and the consecutive ones have isNewElement set to false, because the element has an ID already.

Any ideas on how to combat this?

@strandofgenius
Copy link
Author

This site setup does only have one site listed, but after seeing this I just double checked this site's database and found it has two site groups and the site is assigned to the second group. I am unsure as to why or how that came to be. I'm assuming it could fall under that issue of what @gustavs-gutmanis is talking about?

@gustavs-gutmanis
Copy link

@strandofgenius even if that's the case, it should still be taken care of somehow. Maybe we should not be propagating when saving? But I doubt it's possible, since propagation happens always on new elements.

@khalwat
Copy link
Collaborator

khalwat commented Jan 12, 2023

So perhaps a solution here is that I also check isPropagating?

@gustavs-gutmanis
Copy link

gustavs-gutmanis commented Jan 13, 2023 via email

@strandofgenius
Copy link
Author

@khalwat any update on this?

khalwat added a commit that referenced this issue Feb 6, 2023
…were disabled for a particular section, in certain circumstances ([#1212](#1212))
khalwat added a commit that referenced this issue Feb 6, 2023
…URLs were disabled for a particular section, in certain circumstances ([#1212](#1212))
@khalwat
Copy link
Collaborator

khalwat commented Feb 6, 2023

So I implemented this, but I have a concern -- if I just don't invalidate MetaBundles when the element is propagating, I'm thinking there would be legit cases where it should be invalidating them.

Consider if you save an element that propagates to other sites... in those cases, we do want the metabundles to be invalidated, and we do want the sitemaps to be regenerated.

So I think that's the wrong approach. What I'm doing now is checking whether URLs are disabled for the section via $metaBundle->metaSitemapVars->sitemapUrls

Addressed in 04780df & 00d92b9

Craft CMS 3:

You can try it now by setting your semver in your composer.json to look like this:

    "nystudio107/craft-seomatic": "dev-develop as 3.4.49",

Then do a composer clear-cache && composer update

…..

Craft CMS 4:

You can try it now by setting your semver in your composer.json to look like this:

    "nystudio107/craft-seomatic": "dev-develop-v4 as 4.0.20”,

Then do a composer clear-cache && composer update

@strandofgenius
Copy link
Author

Thank you for your time on this. From what I've tested this resolves the queue jobs being generated if the sections are disabled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants