Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pivica created an issue. See original summary.

Primsi’s picture

Status: Active » Needs review
FileSize
62.54 KB

Initial patch. What was done:

  1. Converted to Commands subclass
  2. Removed deprecated code: drush_log, drush_print and drush_user_abort

The last command needs a bit more testing, didn't do any.

Primsi’s picture

Addding the composer bit that was missing.

Berdir’s picture

Status: Needs review » Needs work

> 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.

  1. +++ b/composer.json
    @@ -10,5 +10,12 @@
    +        "drush.services.yml": "^9"
    

    seems like it is recommended to do ^9 || ^10 now.

  2. +++ b/drush.services.yml
    @@ -0,0 +1,5 @@
    +  .commands:
    

    that should be bs_lib.commands?

  3. +++ b/src/Commands/BsLibCommands.php
    @@ -0,0 +1,1719 @@
    +    // Child theme should not exist.
    +    if (!empty($child_path = $this->drupalGetThemePath($child_machine_name))) {
    +      $this->logger()->error("Child theme already exist on $child_path file system.");
    +      return;
    +    }
    

    From looking at some examples, it's also possible to throw an exception here, but not quite sure how that will look/work.

  4. +++ b/src/Commands/BsLibCommands.php
    @@ -0,0 +1,1719 @@
    +    // Copy files from parent and change/apply text changes to labels.
    +    $this->copyThemeFiles($options);
    +
    +    // Replace text in copied files.
    +    $this->reconfigureThemeFiles($options);
    +
    +    // Generate some files from the scratch.
    +    $this->generateFile('theme-options.yml', $options);
    +    $this->generateFile("config/schema/{$child_machine_name}.schema.yml", $options);
    +    $this->generateFile('gulp-options.yml', $options);
    +    $this->generateFile($child_machine_name . '.info.yml', $options);
    +    $this->generateFile($child_machine_name . '.libraries.yml', $options);
    +    $this->generateFile('README.md', $options);
    +
    

    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.

Primsi’s picture

> 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-...

Berdir’s picture

We 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.

Primsi’s picture

Status: Needs work » Needs review
FileSize
51.82 KB
22.88 KB

Addressing 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.

Primsi’s picture

Primsi’s picture

We 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

Primsi’s picture

Seems 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.

Primsi’s picture

Addressing 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.

pivica’s picture

Why 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?

Primsi’s picture

Yeah, 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.

pivica’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +next-release

Still looks a bit strange:

drush bs-tc primer_theme design_new_1 'Design Test' 'Design Test.'
Starting design_new_1 theme creation
Building CSS asset for a design_new_1 theme
 [ok] Installing any missing package and rebuilding sass files                                                                  
 [ok] Rebuilding CSS files
drush bs-tu primer_demo_theme
Updating a primer_demo_theme theme
No theme updates required.
Building CSS asset for a primer_demo_theme theme
 [ok] Installing any missing package and rebuilding sass files
 [ok] Rebuilding CSS files

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 :)

pivica’s picture

pivica’s picture

Some minor changes in comments while testing changes in #3045552: Evaluate pnpm package manager, nothing important.

pivica’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -next-release

Committed.

pivica’s picture

pivica’s picture

Status: Fixed » Needs work

This is still not committed and the latest patch is failing.

  • pivica committed 1d8cbd4 on 8.x-1.x
    Issue #3071090 by Primsi, pivica, Berdir: Add drush 9 generator support...
pivica’s picture

Status: Needs work » Fixed

And now it is committed.

Status: Fixed » Closed (fixed)

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