Multiple core modules attempt to resolve a route to the block admin page without first checking that the block module is installed, resulting in a PHP exception being thrown (see original issue summary).
Need to wrap route references with module exists calls to prevent exceptions.
Original Issue Summary:
Hello
The Help module seems to be generating errors when the Block module is not installed.
After a fresh install of Drupal 8-dev and immediately uninstalling "Custom Block" and "Block" modules, the modules page (/admin/modules) is not accessible. Following error is shown:
Uncaught PHP Exception Symfony\Component\Routing\Exception\RouteNotFoundException: "Route "block.admin_display" does not exist." at .../drupal-8.0.x-dev/core/lib/Drupal/Core/Routing/RouteProvider.php line 127
Uninstalling Help (or reinstalling Block) module takes away the error.
Comment | File | Size | Author |
---|---|---|---|
#42 | interdiff-2468395-29-42.txt | 12.18 KB | opratr |
#42 | core_modules_reference-2468395-42.patch | 12.4 KB | opratr |
Comments
Comment #1
LinL CreditAttribution: LinL commentedLooks like the help module info.yml file is missing a dependency on the block module. Here's a patch that adds that.
Not sure how to add tests for this?
Comment #3
roderik@ #1: I don't think any tests need to be added explicitly for this.
However, some tests are failing now. They probably assume that block module gets uninstalled at some point, where it does not. (Because now the help module depends on it.)
If someone wants to take this on, they need to read through the tests that are failing, and see if they can tell why that is.
The failing tests are mentioned at the "view (test results)" link with the failed patch (which looks nicer if you've installed Dreditor), so read through the failing lines to see what is failing and why.
...and if you can see that, you can fix it.
Comment #4
lostkangaroo CreditAttribution: lostkangaroo commentedWasn't able to replicate the test failures locally so lets see what happens with this attempt.
Comment #5
lostkangaroo CreditAttribution: lostkangaroo commentedThis needs review, doh!
Comment #7
lostkangaroo CreditAttribution: lostkangaroo commentedOne test down so lets see if this patch finishes this out.
Comment #9
lostkangaroo CreditAttribution: lostkangaroo commentedStopped using shortcuts to determine what the output will be for the assertText test that keeps failing. This latest patch corrects the mistakes of the previous patches.
Comment #11
lostkangaroo CreditAttribution: lostkangaroo commentedNothing like a little reordering of dependent modules to make you go nuts.
Comment #12
lostkangaroo CreditAttribution: lostkangaroo commentedAnd it returns green, the dance party begins now!!
Comment #13
jm.federico CreditAttribution: jm.federico commentedShouldn't the Help module work without Blocks module enabled?
I for instance will be using Drupal 8 a lot without the block module, but still would like the admin pages to show the help messages to users.
Comment #14
jm.federico CreditAttribution: jm.federico commentedMaybe implemente a preprocess_page hook when the module is not enabled and add a $help variable?
Comment #15
lostkangaroo CreditAttribution: lostkangaroo commentedWhile I agree with you in principle that the help module shouldn't be tied to the block module your suggestions change the focus of this issue quite a bit. There is a very tight coupling between the two modules involving the system module that would require some major surgery in both help and system to fully accomplish this. Check https://www.drupal.org/project/issues/drupal?component=help.module and add an issue there if you want to pursue it.
This issue should focus solely on fixing the current bug as it exists in site breaking form without making changes in the other components.
Comment #16
Jody LynnI reproduced the problem and reviewed and tested the patch.
Comment #17
alexpottThe problem here is that
shortcut_help()
has the following line of code:This line creates a dependency on the block module. We should wrap this line in a module exists check.
Comment #18
alexpottComment #19
jcfiala CreditAttribution: jcfiala as a volunteer commentedI'm willing to look at this at the sprint.
Comment #20
dsoini CreditAttribution: dsoini commentedI have looked at this at the sprint. It seems to me that there are dependencies throughout the core modules. For example, if a module has text that searches for the URL of another module, there is a dependency. This issue is larger than just this one set of dependencies.
Comment #21
jcfiala CreditAttribution: jcfiala as a volunteer commentedYeah, I tried doing the change suggested in #17, and it wasn't sufficient to fix the problem - removing the block module still caused the modules screen to fail. I would suggest going back to the original fix, adding blocks to dependencies.
At this point I am not working on this ticket, if it's interesting to someone else.
Comment #22
alexpottSee contact_help() for an example of how to deal with this.
Is the line that takes care of this.
Comment #23
opratr CreditAttribution: opratr as a volunteer commentedI'm at DrupalCon LA 2015 and I'm working on a patch for this issue.
Comment #24
aburrows CreditAttribution: aburrows as a volunteer commentedLooking at this now
Comment #25
aburrows CreditAttribution: aburrows as a volunteer commentedComment #26
aburrows CreditAttribution: aburrows as a volunteer commentedSpeaking with xjm this does cause a bigger issue in the fact that disabling block from core breaks so far this help page and the modules page (admin/modules) from my early testing, but I'm sure no doubt there will be other pages that break..
So we would need to fix all aspects that blocks are required in core. Of course we can create the patches for this without a problem, we just don't want to break core in such a way that will put us back even more. Will work on the patch to fix this though.
Comment #27
opratr CreditAttribution: opratr as a volunteer commentedaburrows,
I have a patch for this I'm testing now. Will post soon. I've wrapped all instances in core modules that attempt to reference the block admin route with a module exists call.
-opratr
Comment #28
opratr CreditAttribution: opratr as a volunteer commentedNeed to consider changing the issue title and summary since this impacts multiple core modules, not just help.
Comment #29
opratr CreditAttribution: opratr as a volunteer commentedComment #30
aburrows CreditAttribution: aburrows as a volunteer commentedTested pages and had no issues RTBC+
Comment #31
alexpottAs per #28 and also the patch in #29 has not been tested for some reason.
Comment #32
alexpottComment #33
opratr CreditAttribution: opratr as a volunteer commentedThanks for the eval aburrows!
Making issue summary changes now
Comment #34
cilefen CreditAttribution: cilefen commentedDo we need a follow-up issue based on #20?
Comment #35
opratr CreditAttribution: opratr as a volunteer commentedUpdated issue summary and meta-data to reflect expanded scope of issue.
Comment #36
opratr CreditAttribution: opratr as a volunteer commentedI think this issue addresses the example issue that #20 refers to, but if we can find other examples of module dependencies that aren't being properly coded for than I would agree that a follow-on issue should be created.
Comment #37
opratr CreditAttribution: opratr as a volunteer commentedBased on feedback from @alexpott I'm refactoring my patch to improve code readability. New patch coming soon...
Comment #38
opratr CreditAttribution: opratr as a volunteer commentedComment #39
geerlingguy CreditAttribution: geerlingguy as a volunteer commentedComment #41
opratr CreditAttribution: opratr as a volunteer commentedNeed to rebase and generate new patch.
Comment #42
opratr CreditAttribution: opratr as a volunteer commentedRebased and created new patch & interdiff
Comment #43
opratr CreditAttribution: opratr as a volunteer commentedComment #44
jhodgdonOn #2473105: Update hook_help texts that link to modules that can be uninstalled, I just committed a patch that mostly duplicates this one (credited people who worked on this)... I wasn't aware of this issue, and apparently the people here weren't aware of that one, but alexpott alerted me.
So, I think this issue is mostly resolved.
Alex suggested that on this issue, we could repurpose it to improve the syntax so it's less in-line.
For instance, if a line of a hook_help() says something like:
we would move the module exists check out to a local variable called something like $block_help and use that in the t() call. A good example of doing this is in the contact module help.
Any takers on that? We would need to update the title and issue summary on this issue.
Comment #45
jhodgdonOtherwise maybe we should just close this issue as a duplicate and open a new one to fix up the syntax.
Comment #46
opratr CreditAttribution: opratr as a volunteer commentedjhodgdon, take a look at my last patch (#42). I believe I just did what you (and allexpott) were suggesting with moving the module existance call to a local variable, etc. So that's already done in this last patch, for at least the scope of this issue.
I'll take a look at your other issue as well.
-opratr
Comment #47
jhodgdonWell the last patch is not going to apply since I just committed the other issue's patch, which had been worked on for more than a month and has been RTBC for ages. But yes, that last patch here is what I'm talking about.
Comment #48
opratr CreditAttribution: opratr as a volunteer commentedTook a look at that other issue. Definitely more comprehensive than this issue. Thanks for the credit jhodgdon!
Yes, my last patch will no longer apply for sure. No worries. I wish I had noticed the issue you were working on before working on this one. It happens.
I suppose I'm good with either re-purposing this issue or creating a new one.
Comment #49
jhodgdonWell since there was already a patch here going in that direction, I think we should continue with it here. Needs a new summary and title though.
Comment #50
opratr CreditAttribution: opratr as a volunteer commentedOK, I can take care of that unless you have one in mind already. Is there guidance on what to include in an issue summary? The issue summary #2473105 was quite comprehensive.
I'll get started on making the code changes.
Comment #51
jhodgdonYeah if we repurpose this issue, the summary and title should just say something about making the module exists tests into local variables, instead of what it says now about not being able to disable modules... probably really a new issue would be better. The people involved here already got credit on the other patch commit ...
Comment #52
opratr CreditAttribution: opratr as a volunteer commentedAgreed. I'll create a new issue then.
Comment #53
opratr CreditAttribution: opratr as a volunteer commentedClosed due to duplicate of #2473105: Update hook_help texts that link to modules that can be uninstalled
Comment #54
jhodgdonWhen you create a new issue, please post a link here and mark it as "related". Thanks! Also please make the parent of the new issue the same as this issue's parent.
Comment #55
opratr CreditAttribution: opratr as a volunteer commentedA new issue has been created to handle refactoring the module-exists checks into local variables for readability. See related issue.