One of the important aspects of recipes is that they need to be able to be re-applied. Currently, Simple Sitemap's default config includes some config entities that don't include all of the necessary data - that's normally not a problem because those missing values get filled in when the config is saved, but it breaks the ability to reapply a recipe that installs Simple Sitemap, because the active config (which has had the missing values filled in) differs from the config coming from the module itself!
Create a recipe that installs Simple Sitemap and imports its default config. Apply it once, then apply it again - the second time, it will fail with an exception.
Simple Sitemap's config entities should including missing langcode and status values.
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #3
phenaproximaComment #4
gbyteI don't see how adding entity keys to configuration that do not exist in the entities themselves solves any problem; rather this seems to be a strange workaround. Neither sitemap nor sitemap type entities have a language code, because a language code does not make sense. Sitemap type entities don't have a status as there is nothing to unpublish. These are custom entity types for a reason - we don't need everything a node provides.
I have not used recipes yet but only taking under account your description, there seems to be more of a problem with those. Please correct me if you feel I'm wrong.
Comment #5
phenaproximaThey do, actually -- all config entities (well, anything that extends
ConfigEntityBase) do. It's defined in the base class. And in fact, one of the shipped entities explicitly hasstatus: falsein 4.x HEAD. There's no way to remove the field unless you change the entity type classes to not extendConfigEntityBase, but that would create more problems than it would solve.That's an accurate assessment. Recipes are currently too strict about comparing the active config with the config coming from a recipe (which includes any extensions installed by that recipe). That's a problem that needs to be fixed, but it's a complex issue that I've only just begun to work on as a recipe system maintainer. Until that's fixed, though, Simple Sitemap cannot be used with recipes that need to be idempotent (which is, frankly, all of them).
Adding missing properties that Simple Sitemap itself may not use, but is in fact responsible for because it extends
ConfigEntityBase, seems harmless. Especially because, if you install Simple Sitemap on a brand new plain Drupal and rundrush cex, you'll see that those entities havelangcodeandstatus. They're there regardless of their usefulness to Simple Sitemap, so the default config might as well reflect that reality. ;)Comment #7
thejimbirch commentedI just ran into this working on the SEO recipe for Drupal CMS/Starshot.
I updated the config files to match the order of what Drupal exports.
I have changed the status back to Needs review as @phenaproxima has provided additional information to the maintainers.
Comment #8
phenaproximaJust as an FYI, this issue is likely to block inclusion of Simple Sitemap in Drupal CMS. :-\
Comment #9
gbyteChanging category to task as this is not a bug in my book.
And we don't want that to happen, do we? :)
Looking at the MR diff config/install/simple_sitemap.sitemap.default.yml seems off - the file seems to have been mistakenly overwritten.
Please fix that and also look at the sub modules, especially simple_sitemap_engines. Does making a recipe with that module create a similar issue?
Thanks, I'll make sure to merge this in a timely fashion.
Comment #10
phenaproximaSelf-assigning to fix this with test coverage, including all submodules. :)
Comment #11
gbyteOff topic, but on the topic of simple_sitemap_engines: To whoever is responsible, maybe it would make sense to include simple_sitemap_engines with its IndexNow functionality into such a Drupal CMS receipe?
Comment #12
phenaproximaAlright - should be all set.
Here's the commit which added failing test coverage (all the modules are accounted for): https://git.drupalcode.org/issue/simple_sitemap-3468413/-/pipelines/263538
And here's the commit which fixed it, to be sure it won't regress in the future: https://git.drupalcode.org/issue/simple_sitemap-3468413/-/pipelines/263542
Anything else needed here?
Comment #14
gbyteThat was quick. Looks good, thanks!