Support from Acquia helps fund testing for Drupal Acquia logo

Comments

WidgetsBurritos created an issue. See original summary.

WidgetsBurritos’s picture

This patch is a minor refactor which should provide basic drush 9 support, although it probably should be thoroughly tested.

Basic file structure was auto generated with "drush generate drush-command-file" and then I just performed some minor modifications based on some feedback I saw here: https://weitzman.github.io/blog/port-to-drush9

bighappyface’s picture

Status: Needs review » Reviewed & tested by the community

+1 LGTM

Testing results:

$ drush --root=./docroot xmlsitemap:rebuild
XML sitemap files rebuilt in 209308.09 ms. Peak memory usage: 132 MB.
$ drush --root=./docroot xmlsitemap:regenerate
 [notice] Finished XML sitemap generation in 0 sec. Memory usage: 56 MB.
XML sitemap files regenerated in 1591.94 ms. Peak memory usage: 56 MB.
$ drush --root=./docroot xmlsitemap:index
No new XML sitemap links to index.
dpi’s picture

Works as expected.

fgm’s picture

Status: Reviewed & tested by the community » Needs work

The patch needs to include a composer.json, as indicated in the source. Without that file, Drush 9.3 does't find the services file.

fgm’s picture

Added composer.json

Also:

- fixed coding standards in the Commands file
- Added the required comments about open route access in routing files

$ time drush xmlsitemap:rebuild
XML sitemap files rebuilt in 353806.68 ms. Peak memory usage: 338 Mo.
 [notice] Message: The sitemap links were rebuilt.

real	5m55.359s
user	3m48.845s
sys	0m11.622s

$ time drush xmlsitemap:regenerate
 [notice] Finished XML sitemap generation in 0 s. Memory usage: 308 Mo.
XML sitemap files regenerated in 21943.94 ms. Peak memory usage: 308 Mo.
 [notice] Message: The sitemaps were regenerated.

real	0m23.719s
user	0m15.598s
sys	0m1.505s

$ time drush xmlsitemap:index
No new XML sitemap links to index.

real	0m0.684s
user	0m0.502s
sys	0m0.138s
fgm’s picture

FWIW, the test errors appear to be unrelated to the Drush issue, but pertain with missing dependencies on "robotstxt" module and incorrect xmlsitemap storage operation with regard to the test.

fgm’s picture

FTR, works with Drush 9.3.0 too.

m4olivei’s picture

Status: Needs review » Needs work

Looks good! Just a couple of nit picks from me. I'll follow up with a patch.

  1. +++ b/drush.services.yml
    @@ -0,0 +1,8 @@
    +    class: \Drupal\xmlsitemap\Commands\XmlsitemapCommands
    

    Can we use the same casing as other existing classes? This should change to XmlSitemapCommands.

  2. +++ b/src/Commands/XmlsitemapCommands.php
    @@ -0,0 +1,131 @@
    +  public function regenerate() {
    ...
    +    $count_before = db_select('xmlsitemap', 'x')->countQuery()->execute()->fetchField();
    

    Should we take the opportunity to update this to use dependency injection rather than a deprecated function?

  3. +++ b/src/Commands/XmlsitemapCommands.php
    @@ -0,0 +1,131 @@
    +    $count_after = db_select('xmlsitemap', 'x')->countQuery()->execute()->fetchField();
    

    Same here.

m4olivei’s picture

Status: Needs work » Needs review
FileSize
3.13 KB
7.45 KB

Here is an updated patch from #6 with interdiff.

hanoii’s picture

Re-rolling #10, it was adding composer.json while it's already there so the patch was failing.

Dave Reid’s picture

FileSize
6.5 KB
2.67 KB

I tested this out and I was having issues running xmlsitemap-index, as the $limit variable was always being set to 0. For some reason calling $configFactory->get() in the constructor was returning a DrushConfig object instead of a normal core Config object, hence batch_limit was never set. If I just leave the config factory alone and read the variable when the command is getting run, it works as expected.

I also re-added the composer.json segment for the drush.service.yml, and added a suggest for drush.

  • Dave Reid committed 5159f17 on 8.x-1.x
    Issue #2968172 by m4olivei, Dave Reid, fgm, WidgetsBurritos, hanoii:...
Dave Reid’s picture

Status: Needs review » Fixed

Tested and confirmed things are working. Committed #12 to 8.x-1.x. Thanks everyone!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.