Make sure the upgrade path works with Drush 8 and Drush 9

Added this to the master plan as well.

Another important thing to fix here is that our validation that aborts drush updatedb is not being picked up by Drush 9.

I'm working on it.

Bumping to major because many people will upgrade directly to Drush 9 when moving to core 8.4.0.

This does 2 things:

- Refactors our requirements checks to be in a service, so all the logic is only in one place, instead of duplicated code all around.
- Implements the validate hook for drush9. However, unfortunately this validate hook is not being picked up yet, I'm not sure if this is a drush9 bug or I'm doing something wrong. (the same mechanism for defining the validate hook can abort other commands, but not the updatedb, for some reason.)

Wow, not sure how that dump got into the patch, but sorry about that!

Just talked to Moshe Weitzman in slack, and he confirmed me that our validate hook in drush9 will not be able to abort the drush updatedb command, because these hooks only fire on commands that fully bootstrap Drupal. He said though, that he would move forward https://github.com/drush-ops/drush/pull/2708. If that is fixed, then at least people using the latest drush will be covered by the original hook_requirements().

So to summarize, we have:
- Misconfigured sites using the UI at /update.php will have the process aborted by our hook_requirements()
- Misconfigured sites using drush updatedb on drush 8 (before that PR is fixed) will be covered by our drush_hook_COMMAND_validate() implementation
- Misconfigured sites using drush updatedb on drush 8 (after that PR is fixed) will be covered by hook_requirements()
- Misconfigured sites using drush updatedb on drush 9 (before that PR is fixed) will not be covered, meaning that if they don't use drush mecu to check requirements are OK before running the updates, the process will break.
- Misconfigured sites using drush updatedb on drush 9 (after that PR is fixed) will be covered by hook_requirements()

I don't think there's much else we can do, apart from emphasizing in the upgrade instructions that some Drush 9 users are really encouraged to check the requirements with drush mecu before going straight to running DB updates.

Gave this a shallow review and found some nitpicks, but overall it looks quite good.

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

   Can we rename this to make it clearer that these are Drush commands, as opposed to Console commands? Maybe media_entity.commands.drush would be a better name? The class, too, could be renamed DrushCommands instead of MediaEntityCommands.

2. +++ b/media_entity.services.yml
   @@ -2,3 +2,5 @@ services:
   +    class: Drupal\media_entity\MediaEntityCliService
   

   Can we just call the class CliService? It's already in the media_entity namespace, so prefixing it with MediaEntity just makes it longer to type. :)

3. +++ b/src/Commands/MediaEntityCommands.php
   @@ -0,0 +1,45 @@
   + * Add commands for drush 9.
   

   Supernit: Drush is a proper noun and should be capitalized.

4. +++ b/src/Commands/MediaEntityCommands.php
   @@ -0,0 +1,45 @@
   +    $checks = \Drupal::service('media_entity.cli')->validateDbUpdateRequirements();
   

   Any chance we can inject the media_entity.cli service?

5. +++ b/src/MediaEntityCliService.php
   @@ -0,0 +1,84 @@
   + * @internal Only to be used by drush commands.
   

   Supernit: s/drush/Drush

6. +++ b/src/MediaEntityCliService.php
   @@ -0,0 +1,84 @@
   +    module_load_include('install', 'media_entity');
   

   I think we could use module_load_install()?

7. +++ b/src/MediaEntityCliService.php
   @@ -0,0 +1,84 @@
   +      $checks['errors'][] = t('The Media Entity 2.x upgrade path only works with Drupal core >= 8.4.x');
   

   Can we use StringTranslationTrait and $this->t()?

8. +++ b/src/MediaEntityCliService.php
   @@ -0,0 +1,84 @@
   +    if (\Drupal::moduleHandler()->moduleExists('media')) {
   

   Let's inject the module handler.

Thanks for reviewing @phenaproxima!

Addressed all points in #10 but:

1:
Isn't having the file name called drush.services.yml and the tag drush.command enough to indicate that this is a place for Drush commands (and not Console ones)? Also for the class naming, I was under the assumption (after reading https://weitzman.github.io/blog/port-to-drush9) that the best practice would be to have a module's command under a class called ModulenameCommands, but maybe I'm wrong. In any case, no strong feelings about any of this, just let me know if you think we really should change it!

This looks pretty good. I'll do some manual testing.

1. +++ b/src/CliService.php
   @@ -0,0 +1,106 @@
   + * @internal Only to be used by Drush commands.
   

   Let's remove the comment, and just leave it marked @internal. It's possible that this will be needed by a Drupal Console version of this command.

2. +++ b/src/Commands/DrushCommands.php
   @@ -0,0 +1,63 @@
   +    if (drupal_get_installed_schema_version('media_entity') >= 8004) {
   

   Are we sure this should be >=, not >?

Addressed #14.1

Re: #14.2, the last update hook for ME 1.x was 8003, so I believe it can be either >= 8004 or > 8003. Changed to the latter because it appears to be more self-explanatory.

Actually what really makes sense is >= 8201, which is where we start our upgrade stuff.

< /nit >

All looks good to me. I have tested it manually and works like a charm :) @see screenshots

drush mecu :

drush updb

Just small NIT / Coding standards feedback :

- I think small forgetted \ When importing a class with "use"
- Namespaced classes/interfaces/traits should be referenced with use statements
- Array closing indentation error(s)
- Short array syntax must be used to define arrays

I found nitpicks. It's what I do. I'd be fine fixing all of these in a follow-up issue, though! I merely want to document them here for posterity.
None of these should block RTBC or commit.

I'm also marking this issue critical because it effectively blocks the upgrade path to core.

1. +++ b/src/CliService.php
   @@ -0,0 +1,106 @@
   +/**
   + * Class CliService.
   + *
   + * @package Drupal\media_entity
   

   We can remove the @package annotation.

2. +++ b/src/CliService.php
   @@ -0,0 +1,106 @@
   +      $checks['passes'][] = $this->t('Successfully checked that the "Media" module is not installed.');
   

   Nit: Can we rephrase this to: 'The contributed "Media" module is not installed.'?

3. +++ b/src/CliService.php
   @@ -0,0 +1,106 @@
   +      $checks['errors'][] = $this->t('In order to run the Media Entity updates, please make sure all modules that provide plugins for Media Entity (or depend on it) have their code updated to their respective 2.x branches. Note that you will probably need to revert to the 1.x branch of the Media Entity module if you want to uninstall existing plugin modules.') . $additional_msg_providers . $additional_msg_dependent;
   

   Can we make this sentence begin with "Before continuing, please make sure..."

4. +++ b/src/CliService.php
   @@ -0,0 +1,106 @@
   +      $checks['passes'][] = $this->t('Successfully checked that all provider plugins and modules depending on media_entity were updated.');
   

   Nit: Let's remove the "Successfully checked that". And let's end this sentence with "are up-to-date."

5. +++ b/src/CliService.php
   @@ -0,0 +1,106 @@
   +        $checks['passes'][] = $this->t('Successfully located the media_entity_generic module.');
   

   Can we rephrase this as "The media_entity_generic module is installed."?

6. +++ b/src/Commands/DrushCommands.php
   @@ -0,0 +1,63 @@
   +  public function __construct(CliService $cliService) {
   

   Nit: Constructor arguments are usually $snake_case.

7. +++ b/src/Commands/DrushCommands.php
   @@ -0,0 +1,63 @@
   +      $this->logger()->warning(dt('Your site has already run the media_entity DB updates. If you believe this is not correct, you should consider rolling back your database to a previous backup and try again.'));
   

   Let's call $this->logger() once, at the top of the method, so that we can re-use the return value. It'll make the method slightly more readable.

@phenaproxima thanks for reviewing!

This should fix all your nits :)

LGTM :) marcoscano++ :D ready to RTBC :O *_*

Patch no longer applies against 8.x-2.x :(

Re-roll of patch on top of media_entity 2.x.

And manual tests processed :

Since it's just a reroll, I feel OK re-RTBCing this.

Thanks, everyone!