Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Changes from 2 commits
b49093a
f9c9470
6bf3f37
8c0ac39
27dc1f8
c2cfb02
e19f301
96df16d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 makes the module uninstallable in the install screen before the requirements is ever reached.
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.
Actually what you have added to
apigee_edge_apidocs_requirements()
is perfectly enough.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:
![image](https://user-images.githubusercontent.com/1755573/58154887-f03a9d80-7c72-11e9-8db8-41a3b3ed7b0b.png)
(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.
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
?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: b49093aThere 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:
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.
@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 theKernelEvents::RESPONSE
event and the 💩 event dispatcher calls the uninstalled Apigee Edge Teams module'sTeamStatusWarningSubscriber
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
, theDeveloperStatusWarningSubscriber
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.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.
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;