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
162 stars 68 forks source link

Sitemaps are not generated when enabling entry in detail page #1388

Closed martinhellwagner closed 7 months ago

martinhellwagner commented 7 months ago

Describe the bug

When enabling / disabling an entry in the list (with Set Status), the Generate Sitemaps queue job is running and the sitemap is generated anew.

Screenshot 2023-11-23 at 15 24 43

However, when enabling / disabling an entry in the entry's detail page, this queue job is not running and the sitemap is not generated. Is this a bug, or intended?

Screenshot 2023-11-23 at 15 25 02

To reproduce

Steps to reproduce the behaviour:

  1. Go to an entry's detail page.
  2. Enable / disable entry.

Expected behaviour

The Generate Sitemaps queue job is running and the sitemap is generated anew when enabling / disabling an entry in the entry's detail page.

Screenshots

See above.

Versions

martinhellwagner commented 7 months ago

@khalwat

Any thoughts on this one?

khalwat commented 7 months ago

I have not looked at it yet, but will shortly.

khalwat commented 7 months ago

Addressed in https://github.com/nystudio107/craft-seomatic/commit/3bbc983fbeb8bc95f69a1c2c70a7371f3f087bb4 & https://github.com/nystudio107/craft-seomatic/commit/6a483e2d8140aabbee1b9a4e1f5ddcf35808d67d

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.67”,

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.36”,

Then do a composer clear-cache && composer update

khalwat commented 7 months ago

Further context:

This would only happen on a multi-site, where the entry is enabled for one site, but disabled for another. This would cause Craft to report back that the entry was "enabled" so it was included in the sitemap.

Now it also checks via getEnabledForSite() on Craft 3.4 and higher.

martinhellwagner commented 7 months ago

@khalwat

I'll check periodically for 4.0.36 to be released.

martinhellwagner commented 7 months ago

@khalwat

I've tested with 4.0.36 – unfortunately it still doesn't work when the entry is enabled / disabled via the entry's detail page. Anything I'm missing?

khalwat commented 7 months ago

It worked as expected in my reproduction case. Can you be certain you're running 4.0.36? If so, some detail on steps to reproduce or additional information about your setup is needed.

martinhellwagner commented 7 months ago

@khalwat

I've double-checked, and I'm indeed running 4.0.36. I've attached two videos – one where the entry is disabled on the overview page (works), and one where the entry is disabled on the detail page (doesn't work).

https://github.com/nystudio107/craft-seomatic/assets/19146752/d6902eab-95cd-461e-981b-98ee75adaf3e

https://github.com/nystudio107/craft-seomatic/assets/19146752/6e8fa3bb-b438-48e9-a38a-5bbb1b2ac2c9

khalwat commented 7 months ago

So here's the same sequence of events as in your second video, but you can see the sitemap being regenerated, and also when I reload the sitemap itself via the URL, it is updated as expected:

https://github.com/nystudio107/craft-seomatic/assets/7570798/f2cbd6f9-5abb-4722-ab6b-8344c912fb2a

So what else could be complicating things here? Did we clear any OPcache on deploy to ensure it's not running the old cached code, despite the new version of the plugin being installed?

Are you able to replicate this issue with the latest SEOmatic in local development?

martinhellwagner commented 7 months ago

@khalwat

That is indeed nearly the same setup as in my example. Only difference I noticed is that your propagations are probably different. This is what the propagations look like for the channel we've tried it in:

Screenshot 2023-12-06 at 09 11 50

Thus, the switch "English ..." is not present is our entries. Disabling / enabling the entry on the detail page as well as on the overview page only disables / enables the entry itself:

Screenshot 2023-12-06 at 09 10 20
khalwat commented 7 months ago

Okay, after spending some time in XDebug, what is happening here is because you have it explicitly set to "Only save entries to the site where they were created in", Craft sets the Element::$scenario to SCENARIO_ESSENTIALS

The problem is that SEOmatic checks the element's scenario, and will skip regenerating the sitemaps if the scenario of the element is set to SCENARIO_ESSENTIALS

It does this because this is the scenario that elements are set to when they are being bulk resaved, or propagated to other sites... which are cases where we do not want to create a whole lot of queue jobs that aren't necessary.

I'm not sure if there is another reasonable way to detect when a sitemap should be regenerated for this particular setup.

So I guess my question would be: how important to you is it that this functions as you're expecting in this particular scenario? I'm a little hamstrung in that I don't want to make changes to address this particular situation that could cause other functionality to break.

martinhellwagner commented 7 months ago

Thanks for getting to the bottom of this! 🙏🏼

Is there a possibility to disable this check with a Composer patch specific to our setup? Where in the code is this check that skips the sitemap generation located?

Unfortunately, it‘s quite important for our client that sitemap generation works in that exact scenario.

khalwat commented 7 months ago

Addressed via: https://github.com/nystudio107/craft-seomatic/commit/1a99baa8f247e2edcc2dc530b898a46c63828ea5 & https://github.com/nystudio107/craft-seomatic/commit/8baa9fa62b37540f8ec4459a15da1e0bbbe8952a

I will need you to test this with your specific setup to ensure it works before I will cut a release.

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.68”,

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.37”,

Then do a composer clear-cache && composer update

martinhellwagner commented 7 months ago

@khalwat

Works like a charm now, thanks so much! 🚀