The current 8.x-1.x version is only compatible with Drush 8. It defines a drush cache hook which is no longer triggered in Drush 9.x and needs to be converted.

This will imply reverting #2864907: Remove composer file since Drush 9 needs a composer.json to load its services file.

CommentFileSizeAuthor
#8 interdiff-2977745-6-8.txt328 bytesfgm
#8 0001-Issue-2977745-by-fgm-add-Drush-9-support-8.patch2.86 KBfgm
#6 0001-Issue-2977745-by-fgm-make-Token-compatible-with-Drush-89.6.patch3.43 KBfgm
#3 0001-Issue-2977745-by-fgm-Support-Drush-9-for-cache-clear.patch3.71 KBfgm

Comments

fgm created an issue. See original summary.

fgm’s picture

fgm
Primary language French
Location Paris, France
commented
Issue summary: View changes
fgm’s picture

fgm
Primary language French
Location Paris, France
commented
Status: Active » Needs review
Issue tags: +Drush-9
Related issues: +#2864907: Remove composer file
StatusFileSize
new 0001-Issue-2977745-by-fgm-Support-Drush-9-for-cache-clear.patch3.71 KB

Suggested patch

webflo’s picture

webflo commented 
+++ b/src/Commands/TokenCommands.php
@@ -0,0 +1,48 @@
+    $types['token'] = \token_clear_cache::class;

+ $types['token'] = \token_clear_cache::class;

This notation for a callable looks odd. token_clear_cache is a function not a class.

fgm’s picture

fgm
Primary language French
Location Paris, France
commented

Yes, PHP syntax has these weird peculiarities. The possible upside is it allows an IDE to find the backreference from the function declaration, whereas using "token_clear_cache" instead of \token_clear_cache::class would not.

fgm’s picture

fgm
Primary language French
Location Paris, France
commented
StatusFileSize
new 0001-Issue-2977745-by-fgm-make-Token-compatible-with-Drush-89.6.patch3.43 KB

Rerolled on current HEAD, and removed the language construct which bothered @webflo.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Hm. token_cache_clear() calls the Drupal core cache clear and resets a bunch of static caches, which is pointless in a drush call.

Meaning, this isn't token.module specific. Maybe this could just be added directly to drush if it's useful (not sure about that?) as it's really a drupal core integration? Then we'd just phase this out and eventually remove it when drush8 is no longer supported?

fgm’s picture

fgm
Primary language French
Location Paris, France
commented
StatusFileSize
new 0001-Issue-2977745-by-fgm-add-Drush-9-support-8.patch2.86 KB
new interdiff-2977745-6-8.txt328 bytes

@berdir: I guess you mean token_clear_cache() ? token_cache_clear() only provides a callback which happens to invoke token_clear_cache() itself.

AIUI The important part of this is the resetInfo() call at the beginning of token_clear_cache() which invalidates the "token_info" tag. Which is something one will want to do when the Drush "@hook on-event cache-clear" is triggered.

But at any rate, this is not specific to this patch: this replicates the behavior already present in Drush 8, so changing it would be off-topic for Drush 9 compatibility, would it not ?

In the meantime, rerolled because it no longer applied on HEAD after a previous fix.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Yes, but that resetInfo() API is in core and not in token module. token.module does provide a lot of additional tokens and the UI, but I'm glad for everything that doesn't need to be in this project.

My argument is that this API change of Drush 9 might be a good point to add it directly to drush instead of having to keep maintaining it here.

neclimdul’s picture

neclimdul
He/Him
commented 
+    if (!$includeBootstrappedTypes || !$this->moduleHandler->moduleExists('token')) {
+      return;
+    }
+
+    $types['token'] = 'token_clear_cache';

Drive by review, it would be simpler if you reversed the if logic and only set the type when everything is true.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work

#2717721: Drush commands for bulk alias updates doesn't need the composer.json part apparently?

fgm’s picture

fgm
Primary language French
Location Paris, France
commented

The composer part used to be necessary in earlier Drush 9.x versions for Drush to find its services file (and allowed it to have a non-default name).

Maybe it stopped being required in 9.5/9.6 ?

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Fixed

I still don't see why it's token that need to add it, but I suppose it doesn't hurt and to be fair, that's true for most what this module does ;)

Plus, I need a composer.json now for D9 compatibility, so cleaned up and removed the non-required parts and commited.

  • Berdir committed af47625 on 8.x-1.x authored by fgm 
    Issue #2977745 by fgm, Berdir, neclimdul: Drush 9 compatibility
fgm’s picture

fgm
Primary language French
Location Paris, France
commented

Thanks @Berdir. Howver, now that Drush 10 is out, it would be simple to make token compatible with Drush 10 by just modifying line 11 of composer.json from

"drush.services.yml": "^9"

to

"drush.services.yml": "^9 || ^10"

Would you commit it that change too, while you're on it ?

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Done.

  • Berdir committed a3d0501 on 8.x-1.x 
    Issue #2977745 by fgm, Berdir, neclimdul: Drush 10 compatibility
fgm’s picture

fgm
Primary language French
Location Paris, France
commented

Now, /that/'s fast :-D

Status: Fixed » Closed (fixed)

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