Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.When using this, we can take advantage of the Drupal caching mechanism. Local testing makes the request about 4 times faster as well.
Haven't figured out how/or when exactly to clear it, but it's a good step forward though.
| Comment | File | Size | Author |
|---|---|---|---|
| #10 | cache-simplesitemap.patch | 5.82 KB | swentel |
| #2 | 2670676-2.patch | 922 bytes | swentel |
Comments
Comment #2
swentel CreditAttribution: swentel commentedAnd the patch
Comment #4
gbyte CreditAttribution: gbyte as a volunteer and commentedJust pushed it, as it didn't introduce functionality problems. If anything, it should speed up the sitemap index, as this is the only thing generated on the fly, however I haven't noticed any speed increse.
There should be no performance improvements if index is turned off. Feel free to submit patches if you feel this implementation is not yet optimal.
Comment #5
swentel CreditAttribution: swentel commentedCool thanks!
Generation itself is fast, and now when caching is enabled, Drupal doesn't need to fully bootstrap when there's a cached version, it can even live in varnish now respecting the TTL, cool!
I'll have a look in integrating with the purge module so that it can invalidated on demand, will post that in a different issue.
Comment #7
gbyte CreditAttribution: gbyte as a volunteer and commentedI had to go back to Response because the cached output sometimes persists after sitemap regeneration.
@swentel Do you happen to know the best way to invalidate the CachedResponse? This invalidation should happen shortly before sitemap generation.
Comment #8
swentel CreditAttribution: swentel commentedI'll have a look at this during Drupalcamp London this weekend :)
Comment #9
gbyte CreditAttribution: gbyte as a volunteer and commentedSounds like lots of fun, I'd join you if I could. Feel free to rip this module apart.
D8 needs some attention as well. E.g the batch api is a mess (hence its messy implementation in 2.x).
Anyway have fun!
Comment #10
swentel CreditAttribution: swentel commentedNew patch, which also includes a test to actually prove that it works :)
I also removed the drupal_set_message() call from the generate_sitemap() method as it doesn't really belong there.
Comment #12
gbyte CreditAttribution: gbyte as a volunteer and commentedGreat stuff!
Pity you didn't write it against 2.x, I ported it to 2.x as well as I do not intend to maintain 1.x for much longer.
Initial tests are successful in both branches, except for a small issue in 1.x where you go to sitemap.xml right after installing the module and an error occurs:
LogicException: The controller result claims to be providing relevant cache metadata, but leaked metadata was detected. Please ensure you are not rendering content too early.This happens only when visiting sitemap.xml having never generated the sitemap. This issue is not present in 2.x. Shell I keep the issue open for you? ;)
Comment #13
swentel CreditAttribution: swentel commentedI had problems getting things configured in the UI in the 2.x branch (got exceptions with plugins, but didn't really look into why), so I switched back to 1.x. Yes leave the issue open so I can trace it down, because I remember having the exception at least once myself too, but couldn't reproduce it anymore afterwards.
Comment #14
swentel CreditAttribution: swentel commentedOtoh, like you say, 2.x branch is probably the best path forward, especially because of the module namespace :)
Let me think about it for a couple of days, I'll let you know.
Comment #15
gbyte CreditAttribution: gbyte as a volunteer and commentedExceptions with plugins? Strange, haven't had any problems with 2.x. It's just important to uninstall and remove the files of 1.x before installing 2.x. If you find you still have a problem, please open a new issue.
The exception in 1.x is reproducible by installing the module and going to sitemap.xml without generating the sitemap.
The only thing that probably won't be working in 2.x is your test, as the batch will start new threads and the code in testSimplesitemap() will not wait for the sitemap's generation.
Thanks for your input.
Comment #16
gbyte CreditAttribution: gbyte as a volunteer and commentedComment #18
tedfordgif CreditAttribution: tedfordgif at WebFirst, Inc. commentedThis doesn't seem to result in the request being cacheable. I think a different approach is needed, as seen in Feed::buildResponse. I'm testing with Drupal 8.2.x. If this rings true in your testing, I'll open a new issue, unless you want to re-open this one.Edit: Disregard, sorry for the noise.