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.
Problem/Motivation
This is a follow-up issue of #3038262: Add support for Drush 9 issue.
We can not add support for bs_base generator for Drush 9+ versions in bs_base because themes are not supported. So we need to add Drush 9 version in this module. In the same time, we will keep the Drush 8 version code in bs_base. In this way, we can support both versions of a Drush.
Comment | File | Size | Author |
---|---|---|---|
#16 | add-drush-9-support-3071090-16.patch | 25.08 KB | pivica |
#16 | interdiff-3071090-13-16.txt | 678 bytes | pivica |
#10 | add-drush-9-support-3071090-10.interdiff.txt | 2.15 KB | Primsi |
#9 | add-drush-9-support-3071090-9.patch | 23.5 KB | Primsi |
#9 | add-drush-9-support-3071090-9.interdiff.txt | 1.15 KB | Primsi |
Comments
Comment #2
Primsi CreditAttribution: Primsi at MD Systems GmbH for MD Systems GmbH commentedInitial patch. What was done:
The last command needs a bit more testing, didn't do any.
Comment #3
Primsi CreditAttribution: Primsi at MD Systems GmbH for MD Systems GmbH commentedAddding the composer bit that was missing.
Comment #4
Berdir> We can not add support for bs_base generator for Drush 9+ versions in bs_base because themes are not supported.
Why not?
radix for example seems to have drush9 integration: https://git.drupalcode.org/project/radix
That said, if we make the code here re-usable then we could have an actual service with the logic and reuse it for drush 8 and 9 and *that* would actually be something only a module can do.
seems like it is recommended to do ^9 || ^10 now.
that should be bs_lib.commands?
From looking at some examples, it's also possible to throw an exception here, but not quite sure how that will look/work.
Yeah, this is duplicating a lot of code, so it would be nice if we could make it re-usable in a separate service, which themes can't provide. we could just load bs_base.drush.inc ourself and call the functions from that? we can still move them in here once we drop drush 8 support.
Comment #5
Primsi CreditAttribution: Primsi at MD Systems GmbH for MD Systems GmbH commented> 4.
Or we could have a service with commands logic too and then call those from both sides? Like described here:
https://nuvole.org/blog/2017/oct/13/how-maintain-drush-commands-drush-8-...
Comment #6
BerdirWe could, but changing the existing could would likely conflict quite a bit with other things, there were already several changes to the code in bs_base since you started with this I think. I'd prefer a minimal approach for now I think, that changes as little as possible for existing code.
Comment #7
Primsi CreditAttribution: Primsi at MD Systems GmbH for MD Systems GmbH commentedAddressing comments. I removed the code from all private methods and replaced with function calls. Or did we want to call the old functions from the command methods directly? I thought having this wrapper would make the move a bit easier later.
Also, some of the utility functions from bs_base use legacy methods like drush_print and drush_log. We will then change that when moving stuff here.
Also changing patch name,... seems like I used the nid from the bs_base issue.
Comment #8
Primsi CreditAttribution: Primsi at MD Systems GmbH for MD Systems GmbH commentedAdding stuff from #3099329: Improve generator theme-options.yml content generation.
Comment #9
Primsi CreditAttribution: Primsi at MD Systems GmbH for MD Systems GmbH commentedWe were able to run commands without db before, adapting so we can do the same now:
- removed bs_lib module validation
- added correct bootstrap level info
Comment #10
Primsi CreditAttribution: Primsi at MD Systems GmbH for MD Systems GmbH commentedSeems like we need to have themeRunUpdateHooks() in here, because the old function calls drush_confirm() which doesn't exist in d9 any more resulting in a fatal.
Comment #11
Primsi CreditAttribution: Primsi at MD Systems GmbH for MD Systems GmbH commentedAddressing xposted review from here: https://www.drupal.org/project/bs_base/issues/3099329#comment-13423917
We can use
$this->output()->writeln()
instead, but we don't get the nice [foo] thingies there. In this patch I am faking them. Not sure if this makes sense, will remove if needed.Comment #12
pivica CreditAttribution: pivica at MD Systems GmbH commentedWhy is faking with writeln better than just using notice callback without any hack? I don't mind if we change report string from info to notice because Drush changed verbosity level.
Also we will move that bs_base drush code here when we remove Drush 8 support and will need to use the same approach as here. Having writeln with string [info] looks strange. Either we use notice call everywhere or we update documentation stating that if you want to see console logs you use drush with -v. I think notice call is better?
Comment #13
Primsi CreditAttribution: Primsi at MD Systems GmbH for MD Systems GmbH commentedYeah, faking it feels silly. Removed.
On the other hand, we have quite a log
$this->output()->writeln(
in the patch. This is not a "hacky" solution. From drush documentation using logger is preffered, but writeln is also possible if the output needs to be seen regardless of verbosity level.We can still go the
logger()->notice
way though, if you think this suits better our needs.Comment #14
pivica CreditAttribution: pivica at MD Systems GmbH commentedStill looks a bit strange:
But i guess now the problem is that calls from bs_base drush functions are using old Drush API for logging. We will fix this in future when we remove support for Drush 8 and move all related code here in bs_lib command. Possibly we will revert most of the stuff to writeln call and use notice call sparingly only when starting and finishing tasks. Created follow-up issue for that #3107100: Move all related generator code to bs_lib from bs_base.
So fine to merge this as it is right now :)
Comment #15
pivica CreditAttribution: pivica at MD Systems GmbH commentedComment #16
pivica CreditAttribution: pivica at MD Systems GmbH commentedSome minor changes in comments while testing changes in #3045552: Evaluate pnpm package manager, nothing important.
Comment #17
pivica CreditAttribution: pivica at MD Systems GmbH commentedCommitted.
Comment #18
pivica CreditAttribution: pivica at MD Systems GmbH commentedComment #19
pivica CreditAttribution: pivica at MD Systems GmbH commentedThis is still not committed and the latest patch is failing.
Comment #21
pivica CreditAttribution: pivica at MD Systems GmbH commentedAnd now it is committed.