[DRUP-803] Real fix for the module uninstall problem + test coverage

[mxr576]

As it revealed the real problem with the module uninstalls in #198 were caused by a Drupal core bug:
#198 (comment)

This PR reverts the unnecessary changes and adds workarounds for the Drupal core. Also contains tests for module uninstalls.

Latest test on Travis CI: https://travis-ci.org/mxr576/apigee-devportal-drupal/builds/538575563

[cnovak]

We don't want to add try/catch blocks just to follow a different pattern than what core is doing. Adding tests are a good idea, but we need to focus on getting features out the door instead of fixing solved problems in different ways.

[mxr576]

We don't want to add try/catch blocks just to follow a different pattern than what core is doing.

@cnovak I think you are misunderstanding what is going on here :) In Drupal core, you may not see this pattern* (which is bad, because hiding object dependencies is anti-pattern) but this problem is not about where do you retrieve entity storages from entity type manager in a class and why.
I know this was a lengthy comment but if you check the video in the end and read the Drupal core issue's description you should understand the change in b49093a

<?php
class ResponseSubscriber implements EventSubscriberInterface {

  /**
   * @var \Drupal\Core\Entity\EntityTypeManagerInterface
   */
  private $entity_type_manager;

  /**
   * ResponseSubscriber constructor.
   *
   * @param \Drupal\Core\Entity\EntityTypeManagerInterface $entityTypeManager
   */
  public function __construct(EntityTypeManagerInterface $entityTypeManager) {
    $this->entity_type_manager = $entityTypeManager;
  }

  /**
   * Does something with foo entities.
   *
   * @param \Symfony\Component\HttpKernel\Event\FilterResponseEvent $event
   *   The event.
   */
  public function handle(FilterResponseEvent $event) {
    $foo_entities = $this->entity_type_manager->getStorage('foo')->loadMultiple();
    // TODO Do something with foo entities.
  }

  /**
   * {@inheritdoc}
   */
  public static function getSubscribedEvents() {
    $events[KernelEvents::RESPONSE][] = ['handle'];
    return $events;
  }

}

and this

<?php
  /**
   * ResponseSubscriber constructor.
   *
   * @param \Drupal\Core\Entity\EntityTypeManagerInterface $entityTypeManager
   */
  public function __construct(EntityTypeManagerInterface $entityTypeManager) {
    $this->fooStorage = $entityTypeManager->getStorage('foo');
  }

  /**
   * Does something with foo entities.
   *
   * @param \Symfony\Component\HttpKernel\Event\FilterResponseEvent $event
   *   The event.
   */
  public function handle(FilterResponseEvent $event) {
    $foo_entities = $this->fooStorage->loadMultiple();
    // TODO Do something with foo entities.
  }

fails exactly the same way with Drupal 8.7. So it does not matter where do you retrieve the entity storage instance, until 3056604 has not been fixed in Drupal core.

Consequently, this change does not fix anything, it just works at this moment because of the TeamStatusWarningSubscriber does not run on the module listing page. Otherwise, it would fail the same way as before.

Compared this with my fix which actually provides a proper temporary workaround for the Drupal core bug which could be easily followed in the new event subscribers and it could be removed when the Drupal core bug gets fixed.

[mxr576]

Fixed failing Apigee Edge uninstall test because of dependencies. New test build: https://travis-ci.org/mxr576/apigee-devportal-drupal/builds/536737890

[mxr576]

@Jaesin Let's continue the discussion about this if you still disagree, shall we?

#198 (comment)

[mxr576]

I have a working patch for the Drupal core bug which is in review at this moment. Added that instead of the workaround that I implemented.

https://travis-ci.org/mxr576/apigee-devportal-drupal/builds/537746931

[mxr576]

The previous test also passed, but I updated to patch so updated this PR as well.
https://travis-ci.org/mxr576/apigee-devportal-drupal/builds/537781480

[mxr576]

Well, interesting. Apigee Edge tests passed with the 18 patch but failed with the 23 from the Drupal issue. Exactly the opposite happened on Drupal.org :) So the perfect location of the event subscriber unregistration should be in-between.

[mxr576]

https://travis-ci.org/mxr576/apigee-devportal-drupal/builds/538575563

[cnovak]

The API Docs module was removed some time back, closing issue