From 65a5da69e843a8d02a200c428690916ecd37ef1e Mon Sep 17 00:00:00 2001 From: Pawel G Date: Fri, 16 Nov 2018 17:27:36 +0100 Subject: [PATCH] Fix empty variants not being deleted on generate and clean up --- .../SitemapGenerator/SitemapGeneratorBase.php | 9 +--- src/Queue/QueueWorker.php | 45 +++++++++++-------- src/Simplesitemap.php | 20 ++------- src/SimplesitemapManager.php | 23 +++++++++- 4 files changed, 53 insertions(+), 44 deletions(-) diff --git a/src/Plugin/simple_sitemap/SitemapGenerator/SitemapGeneratorBase.php b/src/Plugin/simple_sitemap/SitemapGenerator/SitemapGeneratorBase.php index 7576f7a..0645370 100644 --- a/src/Plugin/simple_sitemap/SitemapGenerator/SitemapGeneratorBase.php +++ b/src/Plugin/simple_sitemap/SitemapGenerator/SitemapGeneratorBase.php @@ -119,7 +119,6 @@ abstract class SitemapGeneratorBase extends SimplesitemapPluginBase implements S /** * @return bool - * @todo: Variant cannot be null */ protected function isDefaultVariant() { return $this->sitemapVariant === $this->settings['default_variant']; @@ -128,7 +127,6 @@ abstract class SitemapGeneratorBase extends SimplesitemapPluginBase implements S /** * @param array $links * @return string - * @todo: Variant cannot be null */ abstract protected function getXml(array $links); @@ -146,7 +144,6 @@ abstract class SitemapGeneratorBase extends SimplesitemapPluginBase implements S * Returns the sitemap index for all sitemap chunks of this type. * * @return string - * @todo: Variant cannot be null */ protected function getIndexXml(array $chunk_info) { $this->writer->openMemory(); @@ -183,12 +180,12 @@ abstract class SitemapGeneratorBase extends SimplesitemapPluginBase implements S * @return $this */ public function remove($mode = 'all') { - self::removeSitemapVariants($this->sitemapVariant, $mode); + self::purgeSitemapVariants($this->sitemapVariant, $mode); return $this; } - public static function removeSitemapVariants($variants = NULL, $mode = 'all') { + public static function purgeSitemapVariants($variants = NULL, $mode = 'all') { if (NULL === $variants || !empty((array) $variants)) { $delete_query = \Drupal::database()->delete('simple_sitemap'); @@ -223,7 +220,6 @@ abstract class SitemapGeneratorBase extends SimplesitemapPluginBase implements S * @param array $links * All links with their multilingual versions and settings. * - * @todo Variant cannot be null */ public function generate(array $links) { $highest_id = $this->db->query('SELECT MAX(id) FROM {simple_sitemap}')->fetchField(); @@ -244,7 +240,6 @@ abstract class SitemapGeneratorBase extends SimplesitemapPluginBase implements S /** * @throws \Exception - * @todo: Variant cannot be null */ public function generateIndex() { if (!empty($chunk_info = $this->getChunkInfo()) && count($chunk_info) > 1) { diff --git a/src/Queue/QueueWorker.php b/src/Queue/QueueWorker.php index ff42733..475cbf5 100644 --- a/src/Queue/QueueWorker.php +++ b/src/Queue/QueueWorker.php @@ -115,7 +115,7 @@ class QueueWorker { */ public function deleteQueue() { $this->queue->deleteQueue(); - SitemapGeneratorBase::removeSitemapVariants(NULL, 'unpublished'); //todo should call the remove() method of every plugin instead? + SitemapGeneratorBase::purgeSitemapVariants(NULL, 'unpublished'); $this->variantProcessedNow = NULL; $this->generatorProcessedNow = NULL; $this->results = []; @@ -136,7 +136,7 @@ class QueueWorker { * @todo Lock functionality */ public function rebuildQueue($variants = NULL) { - $data_sets = []; + $all_data_sets = []; $sitemap_variants = $this->manager->getSitemapVariants(); $this->moduleHandler->alter('simple_sitemap_variants', $sitemap_variants); @@ -155,30 +155,38 @@ class QueueWorker { // Adding generate_sitemap operations for all data sets. foreach ($type_definitions[$type]['urlGenerators'] as $url_generator_id) { - foreach ($this->manager->getUrlGenerator($url_generator_id) - ->setSitemapVariant($variant_name) - ->getDataSets() as $data_set) { - - $data_sets[] = [ - 'data' => $data_set, - 'sitemap_variant' => $variant_name, - 'url_generator' => $url_generator_id, - 'sitemap_generator' => $type_definitions[$type]['sitemapGenerator'], - ]; - - if (count($data_sets) === self::REBUILD_QUEUE_CHUNK_ITEM_SIZE) { - $this->queueElements($data_sets); - $data_sets = []; + $data_sets = $this->manager->getUrlGenerator($url_generator_id) + ->setSitemapVariant($variant_name) + ->getDataSets(); + + if (!empty($data_sets)) { + $sitemap_variants[$variant_name]['data'] = TRUE; + foreach ($data_sets as $data_set) { + $all_data_sets[] = [ + 'data' => $data_set, + 'sitemap_variant' => $variant_name, + 'url_generator' => $url_generator_id, + 'sitemap_generator' => $type_definitions[$type]['sitemapGenerator'], + ]; + + if (count($all_data_sets) === self::REBUILD_QUEUE_CHUNK_ITEM_SIZE) { + $this->queueElements($all_data_sets); + $all_data_sets = []; + } } } } } - if (!empty($data_sets)) { - $this->queueElements($data_sets); + if (!empty($all_data_sets)) { + $this->queueElements($all_data_sets); } $this->getQueuedElementCount(TRUE); + // todo: May not be clean to remove sitemap variants data when queuing elements. + // Remove all sitemap variant instances where no results have been queued. + $this->manager->removeSitemap(array_keys(array_filter($sitemap_variants, function($e) { return empty($e['data']); }))); + return $this; } @@ -193,6 +201,7 @@ class QueueWorker { * @throws \Drupal\Component\Plugin\Exception\PluginException * * @todo Lock functionality + * @todo Does not unpublish sitemaps were variant does not have links anymore. */ public function generateSitemap($from = 'form') { diff --git a/src/Simplesitemap.php b/src/Simplesitemap.php index 1642810..d02f9a3 100644 --- a/src/Simplesitemap.php +++ b/src/Simplesitemap.php @@ -299,25 +299,9 @@ class Simplesitemap { /** * @return $this * @throws \Drupal\Component\Plugin\Exception\PluginException - * - * @todo document */ public function removeSitemap() { - $saved_variants = $this->manager->getSitemapVariants(); - $this->moduleHandler->alter('simple_sitemap_variants', $saved_variants); - $remove_variants = NULL !== ($variants = $this->getVariants(FALSE)) - ? array_intersect_key($saved_variants, array_flip((array) $variants)) - : $saved_variants; - - if (!empty($remove_variants)) { - $type_definitions = $this->manager->getSitemapTypes(); - $this->moduleHandler->alter('simple_sitemap_types', $type_definitions); - foreach ($remove_variants as $variant_name => $variant_definition) { - $this->manager->getSitemapGenerator($type_definitions[$variant_definition['type']]['sitemapGenerator']) - ->setSitemapVariant($variant_name) - ->remove(); - } - } + $this->manager->removeSitemap($this->getVariants(FALSE)); return $this; } @@ -326,6 +310,8 @@ class Simplesitemap { * @param string $from * @return $this * @throws \Drupal\Component\Plugin\Exception\PluginException + * + * @todo variants */ public function generateSitemap($from = 'form') { switch($from) { diff --git a/src/SimplesitemapManager.php b/src/SimplesitemapManager.php index 181e050..b1c8c4e 100644 --- a/src/SimplesitemapManager.php +++ b/src/SimplesitemapManager.php @@ -133,7 +133,6 @@ class SimplesitemapManager { * @param null $sitemap_type * @return array * - * @todo document * @todo translate label */ public function getSitemapVariants($sitemap_type = NULL, $attach_type_info = TRUE) { @@ -210,9 +209,29 @@ class SimplesitemapManager { return $this; } + public function removeSitemap($variant_names = NULL) { + $saved_variants = $this->getSitemapVariants(); + $remove_variants = NULL !== $variant_names && !empty((array) $variant_names) + ? array_intersect_key($saved_variants, array_flip((array) $variant_names)) + : $saved_variants; + + if (!empty($remove_variants)) { + $type_definitions = $this->getSitemapTypes(); + foreach ($remove_variants as $variant_name => $variant_definition) { + $this->getSitemapGenerator($type_definitions[$variant_definition['type']]['sitemapGenerator']) + ->setSitemapVariant($variant_name) + ->remove(); + } + } + + return $this; + } + public function removeSitemapVariants($variant_names = NULL) { if (NULL === $variant_names || !empty((array) $variant_names)) { - SitemapGeneratorBase::removeSitemapVariants($variant_names); //todo should call the remove() method of every plugin instead? + + // Remove sitemap instances. + $this->removeSitemap($variant_names); if (NULL === $variant_names) { // Remove all variants and their bundle settings. -- GitLab