As we have figured out before, and pounard realized we still have the drush integration as part of views.

Let's move this out soon.

Comments

catch’s picture

Should this be against Drush?

dawehner’s picture

Project:Views» Drush
Version:8.x-3.x-dev»
Component:Code» User Commands

Good question, moving code between projects is always not clear what to do, but yeah drush fits better.

moshe weitzman’s picture

Sure, I'll take this into Drush core. I looked at the current Views Drush code and it needs

  1. Support --simulate by running config->set and other "destructive" code through drush_op().
  2. Add completion support for arguments
  3. return your data in addition to printing it for the benefit of --backend calls
  4. When declaring options, you can state if a value is optional and give an example value. For example, see options at config-set command at http://api.drush.org/api/drush/commands!core!config.drush.inc/function/c...
damiankloip’s picture

Assigned:Unassigned» damiankloip
Issue tags:-Novice

@moshe weitzman Thanks! I will check out your pointers later and try to get a patch together soon. api.drush.org has been updated too :)

damiankloip’s picture

Status:Active» Needs review
StatusFileSize
new12.45 KB

I think this covers the points in #3.

damiankloip’s picture

Assigned:damiankloip» Unassigned
moshe weitzman’s picture

Status:Needs review» Needs work

Thanks. Some more tweaks ...

You can remove the hook_drush_help() entries except the meta: ones. When missing, Drush uses the description in the command definition which is sufficient here.

When you use those options that are declared with arrays, a value is required so change value' => 'optional', to 'required'.

'example-value' => 'enable',. typo: enabled. or at least thats what the other help lines use.

drush_views_analyze should return its data in addition to printing.

drush_views_disable and drush_views_enable have empty argument handling but thats now done by the command definition. Please remove.

The list and analyze commands should support the --format option. See drush_config_list() for an example. Remember to add this option to the command definitions as well.

damiankloip’s picture

Status:Needs work» Needs review
StatusFileSize
new5.73 KB
new12.52 KB

Thanks for another review. I think I've addressed all those.

It's funny, because I actually wrote the drush_config_list command! so I should have seen that coming.

moshe weitzman’s picture

Status:Needs review» Reviewed & tested by the community

Looks good to me. WIll commit this in a day or two. More feedback is welcome.

damiankloip’s picture

Great, thanks.

moshe weitzman’s picture

Status:Reviewed & tested by the community» Fixed

Committed to master. Will be in 6.x release of Drush. Thanks.

greg.1.anderson’s picture

Status:Fixed» Needs work

Is Drush 6.x for Drupal-8 only? The commit in #11 breaks Drush for Drupal 7.x with Views enabled. If we want to support Drupal 7 &/or Drupal 6 in Drush 6, then we'd need a mechanism to avoid including commands/core/views.drush.inc for Drupal < 8. In the short term, I think that D6 & D7 support in master is desirable, but long-term that might not be practical. If D6 & D7 are unsupported, then we should fail-fast in the bootstrap.

greg.1.anderson’s picture

I guess the other question I have is, would it hurt to put views.drush.inc in Drupal 8 core? Then the right version would be selected automatically.

moshe weitzman’s picture

Category:task» bug
Status:Needs work» Active

It wouldn't hurt, but core has always had a habit of pretending like Contrib doesn't exist in order not to play favorites (or something). It would be fantastic if we could move lots of drush commands to core Drupal but I think thats unlikely.

Drush 6 still want to be compatible with D6 and D7 so this is a bug.

damiankloip’s picture

I agree with moshe, drush command files moving to core is very unlikely. Could we have some logic where the list of command files are fetched that are included; So if there is a command file of the same name in contrib it can overrride the core file before it is included? Maybe it would be in command.inc, I'm not sure without having a proper look.

EDIT: or can we do something with version numbers to include the correct files? Like 'views_8.drush.inc'.

greg.1.anderson’s picture

Status:Active» Needs review
StatusFileSize
new4.32 KB

Wrote my patch before I read #15; made the pattern "views.d8.drush.inc". (The suggestion for contrib overriding Drush core won't work without a large change to the Drush bootstrap, as the Drush core commandfiles are included before Drupal is even bootstrapped.)

Note that the requisite rename of "views.drush.inc" to "views.d8.drush.inc" is not included in this patch. Only lightly tested -- works with Drupal 7. :)

moshe weitzman’s picture

So using drush6 and drupal7 will cause the version specific commandfile to load and the non-version specific will not load? I guess thats a better solution than requiring drush commandfiles to work for all supported drupal versions (i.e. edit views.drush.inc so it works for d6 and d7). is it worth coming up with a syntax like views.d8+.drush.inc in order to signify that the commanddfile should work for drupal9 as well? we'd want to same for drush major version-specific config/alias/commandfiles.

greg.1.anderson’s picture

Status:Needs review» Needs work

In d6 and d7, views.d8.drush.inc in Drush core will not load, and whatever Drush include file is in the View module will load. In d8, the views.d8.drush.inc will load, and there will be no non-version specific file in the Views module. I have not tested what will happen if there are two version-specific files under consideration, but I expect it will work.

I started to support views.d8+.drush.inc (in regex only), but I think it's perhaps not worth it. The easy workaround would be to make views.d9.drush.inc include views.d8.drush.inc if they were compatible.

Be sure to run drush cc drush before applying the patch above, since I changed the format of the commandfile cache. I did not test what happens if you don't do this; probably all commands fail with php errors. Might be best to put the cache format back the way it was, or change the cache filename pattern so the two can never conflict.

greg.1.anderson’s picture

So, I think if we do not support views.d8+.drush.inc, then we can reduce our considerations to just X.drush.inc and X.dMAJOR.drush.inc, and simplify #16 quite a bit.

greg.1.anderson’s picture

Status:Needs work» Needs review
StatusFileSize
new1.63 KB

Here, this is simpler. Again, remember to git mv commands/core/views.drush.inc commands/core/views.d8.drush.inc and drush cc drush before testing.

damiankloip’s picture

For me, this patch seems to be working for d7 but not for d8 sites. Does D8 work for you? Otherwise, it looks good. If/when everyone agrees I guess the file views command file can be renamed in this patch too?

greg.1.anderson’s picture

Status:Needs review» Needs work

Haven't gotten to d8 testing yet. In theory, if the Drupal version does not match, the commandfile should go in the 'deferred' state, and be tried again at a later phase. I think this worked in #16 (but again, didn't really test it thoroughly), but it looks like I probably broke that in #20. Will test more as I have time.

greg.1.anderson’s picture

Status:Needs work» Needs review
StatusFileSize
new2.72 KB

Still haven't tried on d8, but I made myself a "foo.d7.drush.inc" file and tested successfully with d7, fixing a problem that existed in #20 and #16. Should work on d8 too.

jhedstrom’s picture

I haven't tested in detail but the patch in #23still results in a fatal error on both 6 and 7 sites that have views installed when running drush commands.

PHP Fatal error:  Cannot redeclare views_drush_help() (previously declared in /.../drush/commands/core/views.drush.inc:14) in /.../sites/all/modules/views/drush/views.drush.inc on line 28

greg.1.anderson’s picture

To save space, I omitted the rename of views.drush.inc in #23. To test it, run drush cc drush, apply the patch, then git mv commands/core/views.drush.inc commands/core/views.d8.drush.inc. I could provide a formatted patch with the rename if desired.

tim.plunkett’s picture

StatusFileSize
new2.9 KB

Here it is with the moved file, just to make it easier for testers.

moshe weitzman’s picture

Status:Needs review» Reviewed & tested by the community

This can go in whenever Greg is satisfied with it. Also, needs docs wherever we talk about commandfile naming.

greg.1.anderson’s picture

Committed #23 + some docs. d8 install failed for me, so did not test there; will try again later. Re-open if any problems are found.

greg.1.anderson’s picture

Status:Reviewed & tested by the community» Fixed
greg.1.anderson’s picture

Oh, committed to 6 only; presume we don't want this in 5. Should be backwards-compatible enough to do so if desired, though.

greg.1.anderson’s picture

Status:Fixed» Active

Now I am having trouble with 'dl', and git bisect says it was my commit. :(

greg.1.anderson’s picture

Status:Active» Fixed

Fixed with commit 06b1302.

There is a flaw in drush_get_context; I will discuss this in a separate issue later.

greg.1.anderson’s picture

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