-
Notifications
You must be signed in to change notification settings - Fork 44
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[DRUP-803] Deprecate apidocs module #198
[DRUP-803] Deprecate apidocs module #198
Conversation
…dule from uninstalling properly. The container get’s rebuild after a module is uninstalled and it is important that the `entity_type` cache does not get rebuilt while the module being uninstalled is still discoverable.
@@ -8,3 +8,4 @@ dependencies: | |||
- drupal:text | |||
- drupal:file | |||
- apigee_edge:apigee_edge | |||
- apigee_edge:deprecated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, this is rather seems to be some kind of hack then a proper solution ;S Have you seen this doing by other modules?
What I originally suggested on the sprint planning call is an "install" phase check in apigee_edge_apidocs_requirements()
that does not allow to enable the module anymore and displays a deprecation message. Isn't that possible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Meh... so there is an issue with Drush... It does not run hook_requirements()
.
drush-ops/drush#3669
@Jaesin If we do not find a better workaround for this problem, please document in the info.yml with a comment why this "hack" was needed. Preferably with a deep link to the related Drush issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The drawback of the info.yml trick is this:
(and the problem is the same with Drush).
Another potential solution that I could figure out is this. The possible drawback of this solution that module gets installed partially with Drush so we have to uninstall it.
<?php
/**
* Implements hook_module_preinstall().
*/
function apigee_edge_apidocs_module_preinstall($module) {
if (PHP_SAPI === 'cli' && $module === 'apigee_edge_apidocs') {
// Workaround for a bug in Drush: https://github.com/drush-ops/drush/issues/3669
\Drupal::service('module_installer')->uninstall(['apigee_edge_apidocs']);
throw new RuntimeException('The Apigee Edge Apidocs module is deprecated and will be removed before Apigee Edge version 8.x-1.0 is released. Please use the Apigee API Catalog module as an alternative: https://www.drupal.org/project/apigee_api_catalog.');
}
}
cli-drupal:/app$ drush pmu apigee_edge_apidocs -y
[success] Successfully uninstalled: apigee_edge_apidocs
cli-drupal:/app$ drush en apigee_edge_apidocs -y
In apigee_edge_apidocs.module line 53:
The Apigee Edge Apidocs module is deprecated and will be removed before Apigee Edge version 8.x-1.0 is released. Please use the Apigee API Catalog module as an alternative: https://www.drupal.org/project/apigee_
api_catalog.
pm:enable [-h|--help] [-q|--quiet] [-v|vv|vvv|--verbose] [-V|--version] [--ansi] [--no-ansi] [-n|--no-interaction] [-d|--debug] [-y|--yes] [--no] [--remote-host REMOTE-HOST] [--remote-user REMOTE-USER] [-r|--root ROOT] [-l|--uri URI] [--simulate] [--pipe] [-D|--define DEFINE] [--druplicon] [--xh-link XH-LINK] [--notify [NOTIFY]] [--] <command> [<modules>]...
cli-drupal:/app$ drush pm:list | grep apigee_edge_apidocs
Apigee (Deprecated) Apigee Edge API Docs (apigee_edge_apidocs) Disabled 8.x-1.0-rc4
At least Drupal Console calls hook_requirements()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does drush 9 still try to install the module if you keep - apigee_edge:deprecated
?
@@ -8,3 +8,4 @@ dependencies: | |||
- drupal:text | |||
- drupal:file | |||
- apigee_edge:apigee_edge | |||
- apigee_edge:deprecated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, this is rather seems to be some kind of hack then a proper solution ;S Have you seen this doing by other modules?
What I originally suggested on the sprint planning call is an "install" phase check in apigee_edge_apidocs_requirements()
that does not allow to enable the module anymore and displays a deprecation message. Isn't that possible?
@@ -100,9 +86,7 @@ class TeamStatusWarningSubscriber implements EventSubscriberInterface { | |||
public function __construct(AccountInterface $current_user, RouteMatchInterface $route_match, EntityTypeManagerInterface $entity_type_manager, TeamMembershipManagerInterface $team_membership_manager, MessengerInterface $messenger, TranslationInterface $string_translation) { | |||
$this->routeMatch = $route_match; | |||
$this->currentUser = $current_user; | |||
$this->teamStorage = $entity_type_manager->getStorage('team'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are unrelated changes to the PR, please remove them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is related as it prevents a clean uninstall of the apigee_edge_apidocs
module. Accessing methods that access entity type discovery in a service constructor prevents clearing the entity type cache. See the following commit message: b49093a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I remove @Jaesin 's changes I get the following error on uninstall, so these changes need to be in here:
The website encountered an unexpected error. Please try again later.</br></br><em class="placeholder">Drupal\Component\Plugin\Exception\PluginNotFoundException</em>: The "apidoc" entity type does not exist. in <em class="placeholder">Drupal\Core\Entity\EntityTypeManager->getDefinition()</em> (line <em class="placeholder">150</em> of <em class="placeholder">core/lib/Drupal/Core/Entity/EntityTypeManager.php</em>). <pre class="backtrace">Drupal\Core\Entity\EntityTypeManager->getHandler('apidoc', 'views_data') (Line: 157)
Drupal\Core\Entity\EntityManager->getHandler('apidoc', 'views_data') (Line: 177)
views_views_data()
call_user_func_array('views_views_data', Array) (Line: 392)
Drupal\Core\Extension\ModuleHandler->invoke('views', 'views_data') (Line: 243)
Drupal\views\ViewsData->getData() (Line: 160)
Drupal\views\ViewsData->get('apidoc') (Line: 91)
Drupal\views\Plugin\Derivative\ViewsEntityRow->getDerivativeDefinitions(Array) (Line: 101)
Drupal\Component\Plugin\Discovery\DerivativeDiscoveryDecorator->getDerivatives(Array) (Line: 87)
Drupal\Component\Plugin\Discovery\DerivativeDiscoveryDecorator->getDefinitions() (Line: 284)
Drupal\Core\Plugin\DefaultPluginManager->findDefinitions() (Line: 175)
Drupal\Core\Plugin\DefaultPluginManager->getDefinitions() (Line: 147)
views_theme(Array, 'module', 'views', 'core/modules/views') (Line: 447)
Drupal\Core\Theme\Registry->processExtension(Array, 'views', 'module', 'views', 'core/modules/views') (Line: 334)
Drupal\Core\Theme\Registry->build() (Line: 233)
Drupal\Core\Theme\Registry->get() (Line: 86)
Drupal\Core\Utility\ThemeRegistry->initializeRegistry() (Line: 67)
Drupal\Core\Utility\ThemeRegistry->__construct('theme_registry:runtime:seven', Object, Object, Array, 1) (Line: 253)
Drupal\Core\Theme\Registry->getRuntime() (Line: 142)
Drupal\Core\Theme\ThemeManager->render(Array, Array) (Line: 437)
Drupal\Core\Render\Renderer->doRender(Array, ) (Line: 195)
Drupal\Core\Render\Renderer->render(Array, ) (Line: 226)
Drupal\Core\Render\MainContent\HtmlRenderer->Drupal\Core\Render\MainContent\{closure}() (Line: 582)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 227)
Drupal\Core\Render\MainContent\HtmlRenderer->prepare(Array, Object, Object) (Line: 117)
Drupal\Core\Render\MainContent\HtmlRenderer->renderResponse(Array, Object, Object) (Line: 90)
Drupal\Core\EventSubscriber\MainContentViewSubscriber->onViewRenderArray(Object, 'kernel.view', Object)
call_user_func(Array, Object, 'kernel.view', Object) (Line: 111)
Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch('kernel.view', Object) (Line: 156)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 68)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 57)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 106)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 85)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 52)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel->handle(Object, 1, 1) (Line: 693)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)
</pre>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would have been great if someone had validated #141 before this got merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A note about services that access the entity type manager in their constructors. Of course it's OK to inject the entity type manager in a service constructor but accessing any methods that result in
getDefinitions
being called should not be used in a service constructor. When a module is uninstalled, the last thing that happens is the service container is rebuilt. This happens when the module that is being uninstalled is still discoverable. Any entities the module defines will be cached whengetDefinitions
is called on the entity type manager.This results in a
PluginNotFoundException
exception for subsequent page requests. The only way to get back to a working site is to clear the cache which would have to happen through drush. That is fine for developers but it is a problem for anyone that doesn't have drush or the correct alias set up.
@Jaesin Thanks for the investigation and the detailed answer but you haven't captured the real root of the problem. Generally speaking, there should not be any problem with retrieving an instance of a class from an object repository (EntityTypeManager) in a constructor - unless there is a bug somewhere in the system. This is what I tried to convince you in #140 and #141 and this is the reason why I continued my investigation regarding this problem today. Based on my findings this Drupal core issue causes the problem: https://www.drupal.org/project/drupal/issues/3056604
When you uninstall the Apigee Edge Teams module then it gets uninstalled, you can see in the logs: apigee_edge_teams module uninstalled.
. After that HTTP Kernel triggers the KernelEvents::RESPONSE
event and the 💩 event dispatcher calls the uninstalled Apigee Edge Teams module's TeamStatusWarningSubscriber
event subscriber, but it should not.
(Stack trace captured after the Teams module uninstalled and the request has not been resolved. This confirms that the TeamStatusWarningSubscriber
gets called because it is still registered in that instance of the event dispatcher and it fails because the "team" and "team_app" entity types have been unregistered already.)
Update: POC 2, what you changed only works because the TeamStatusWarningSubscriber
only listening on team app routes at this moment. If you remove that condition from event subscriber you still get a WSOD.
https://drive.google.com/open?id=1jQKtoMerpQXvuRQtQ_2KReJVl7u62lu1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found the same thing. Besides the TeamStatusWarningSubscriber
, the DeveloperStatusWarningSubscriber
causes the same issue. The teams module wasn't installed in the case of the error described in this issue.
This is not likely to get fixed in Drupal core any time soon (see: https://www.drupal.org/project/drupal/issues/3056604#comment-13118160). It is true that this results from issues in Drupal core but to be practical, we should work around it by not accessing methods that would cause the entity type cache to be rebuilt when the container is built. There aren't any services in Drupal core that use this pattern even through the entity type manager is used extensively in services so we should avoid it as well. Although the problem is showing itself while uninstalling modules, there could be edge cases where it shows up again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But entity type cache is not being rebuilt when you call $entity_type_manager->getStorage()
in the event's constructor... it had been rebuilt when the module uninstalled (this is what this method does) and after that the only problem that Drupal core calls an uninstalled event dispatcher which tries to retrieve an uninstalled entity type. It does not matter where it tries to retrieve it, in its constructor or in the method that handles the event, it fails. (It does not even matter that this is an event subscriber, any code that runs in the same page request after a module has been uninstalled and it would try to retrieve an uninstalled entity type it would fail.) This is what the video above confirms.
This is not likely to get fixed in Drupal core any time soon
I'll work on this, just as I worked on the JSON API related bugs to ensure this gets fixed sooner as you would expect.
Let's continue this conversation in #203.
@Jaesin Not sure that b49093a is the right resolution of the problem. I do not see any issue with the changed lines. Other contrib modules do the same and you can not alter them in a way like this. It is odd that we haven't seen this issue before, couldn't it be an API docs module related problem? How can I reproduce this issue? I tried, but I could not: Did you have any MINT module enabled? |
To reproduce the |
Using With drush 8, I have the following result when I remove
A note about services that access the entity type manager in their constructors. Of course it's OK to inject the entity type manager in a service constructor but accessing any methods that result in This results in a |
@Jaesin This is what I did [here] exactly(#198 (comment)) 🙄Could you attach a test to this PR that confirms this bug exists and this is the right fix for that? IIRC we had a test that uninstall the Apigee Edge module to confirm that there is no issue like apigee/apigee-m10n-drupal#75. I could not find that at this moment. |
I am using Drupal 8.7.1. with no other modules installed and the Apigee Edge credentials are not set. This is just after Related: #140 |
Which is resolved by #141. Sorry it is too late here, I'll try to reproduce it again tomorrow morning. Did you used the standard installation profile? If you have some time, could you check whether this occurs with rc4 also? |
I changed the update phase to use Drush also shows:
|
@@ -100,9 +86,7 @@ class TeamStatusWarningSubscriber implements EventSubscriberInterface { | |||
public function __construct(AccountInterface $current_user, RouteMatchInterface $route_match, EntityTypeManagerInterface $entity_type_manager, TeamMembershipManagerInterface $team_membership_manager, MessengerInterface $messenger, TranslationInterface $string_translation) { | |||
$this->routeMatch = $route_match; | |||
$this->currentUser = $current_user; | |||
$this->teamStorage = $entity_type_manager->getStorage('team'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I remove @Jaesin 's changes I get the following error on uninstall, so these changes need to be in here:
The website encountered an unexpected error. Please try again later.</br></br><em class="placeholder">Drupal\Component\Plugin\Exception\PluginNotFoundException</em>: The "apidoc" entity type does not exist. in <em class="placeholder">Drupal\Core\Entity\EntityTypeManager->getDefinition()</em> (line <em class="placeholder">150</em> of <em class="placeholder">core/lib/Drupal/Core/Entity/EntityTypeManager.php</em>). <pre class="backtrace">Drupal\Core\Entity\EntityTypeManager->getHandler('apidoc', 'views_data') (Line: 157)
Drupal\Core\Entity\EntityManager->getHandler('apidoc', 'views_data') (Line: 177)
views_views_data()
call_user_func_array('views_views_data', Array) (Line: 392)
Drupal\Core\Extension\ModuleHandler->invoke('views', 'views_data') (Line: 243)
Drupal\views\ViewsData->getData() (Line: 160)
Drupal\views\ViewsData->get('apidoc') (Line: 91)
Drupal\views\Plugin\Derivative\ViewsEntityRow->getDerivativeDefinitions(Array) (Line: 101)
Drupal\Component\Plugin\Discovery\DerivativeDiscoveryDecorator->getDerivatives(Array) (Line: 87)
Drupal\Component\Plugin\Discovery\DerivativeDiscoveryDecorator->getDefinitions() (Line: 284)
Drupal\Core\Plugin\DefaultPluginManager->findDefinitions() (Line: 175)
Drupal\Core\Plugin\DefaultPluginManager->getDefinitions() (Line: 147)
views_theme(Array, 'module', 'views', 'core/modules/views') (Line: 447)
Drupal\Core\Theme\Registry->processExtension(Array, 'views', 'module', 'views', 'core/modules/views') (Line: 334)
Drupal\Core\Theme\Registry->build() (Line: 233)
Drupal\Core\Theme\Registry->get() (Line: 86)
Drupal\Core\Utility\ThemeRegistry->initializeRegistry() (Line: 67)
Drupal\Core\Utility\ThemeRegistry->__construct('theme_registry:runtime:seven', Object, Object, Array, 1) (Line: 253)
Drupal\Core\Theme\Registry->getRuntime() (Line: 142)
Drupal\Core\Theme\ThemeManager->render(Array, Array) (Line: 437)
Drupal\Core\Render\Renderer->doRender(Array, ) (Line: 195)
Drupal\Core\Render\Renderer->render(Array, ) (Line: 226)
Drupal\Core\Render\MainContent\HtmlRenderer->Drupal\Core\Render\MainContent\{closure}() (Line: 582)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 227)
Drupal\Core\Render\MainContent\HtmlRenderer->prepare(Array, Object, Object) (Line: 117)
Drupal\Core\Render\MainContent\HtmlRenderer->renderResponse(Array, Object, Object) (Line: 90)
Drupal\Core\EventSubscriber\MainContentViewSubscriber->onViewRenderArray(Object, 'kernel.view', Object)
call_user_func(Array, Object, 'kernel.view', Object) (Line: 111)
Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch('kernel.view', Object) (Line: 156)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 68)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 57)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 106)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 85)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 52)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel->handle(Object, 1, 1) (Line: 693)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)
</pre>
@@ -135,8 +128,6 @@ public function __construct(ModuleHandlerInterface $module_handler, ClassResolve | |||
$this->moduleHandler = $module_handler; | |||
$this->classResolver = $class_resolver; | |||
$this->teamMembershipManager = $team_membership_manager; | |||
$this->teamRoleStorage = $entity_type_manager->getStorage('team_role'); | |||
$this->teamMemberRoleStorage = $entity_type_manager->getStorage('team_member_role'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is missing $this->entityTypeManager = $entity_type_manager;
No description provided.