Core modules reference block module without first ensuring existance

Looks 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?

@ #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.

Wasn't able to replicate the test failures locally so lets see what happens with this attempt.

This needs review, doh!

One test down so lets see if this patch finishes this out.

Stopped 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.

Nothing like a little reordering of dependent modules to make you go nuts.

And it returns green, the dance party begins now!!

While 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.

I reproduced the problem and reviewed and tested the patch.

The problem here is that shortcut_help() has the following line of code:

      $output .= '<dd>' . t('You can display your shortcuts by enabling the <em>Shortcuts</em> block on the <a href="!blocks">Blocks administration page</a>. Certain administrative modules also display your shortcuts; for example, the core <a href="!toolbar-help">Toolbar module</a> provides a corresponding menu item.', array('!blocks' => \Drupal::url('block.admin_display'), '!toolbar-help' => \Drupal::url('help.page', array('name' => 'toolbar')))) . '</dd>';

This line creates a dependency on the block module. We should wrap this line in a module exists check.

I'm willing to look at this at the sprint.

I 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.

Yeah, 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.

See contact_help() for an example of how to deal with this.

$block_page = \Drupal::moduleHandler()->moduleExists('block') ? \Drupal::url('block.admin_display') : '#';

Is the line that takes care of this.

I'm at DrupalCon LA 2015 and I'm working on a patch for this issue.

Looking at this now

Speaking 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.

aburrows,

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

Need to consider changing the issue title and summary since this impacts multiple core modules, not just help.

Tested pages and had no issues RTBC+

As per #28 and also the patch in #29 has not been tested for some reason.

Thanks for the eval aburrows!
Making issue summary changes now

Do we need a follow-up issue based on #20?

Updated issue summary and meta-data to reflect expanded scope of issue.

I 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.

Based on feedback from @alexpott I'm refactoring my patch to improve code readability. New patch coming soon...

Need to rebase and generate new patch.

Rebased and created new patch & interdiff

On #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:

t('Blah blah <a href="%block">Block module help</a>', array('%block' => module exists block blah blah)

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.

Otherwise maybe we should just close this issue as a duplicate and open a new one to fix up the syntax.

jhodgdon, 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

Well 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.

Took 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.

Well 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.

OK, 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.

Yeah 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 ...

Agreed. I'll create a new issue then.

Closed due to duplicate of #2473105: Update hook_help texts that link to modules that can be uninstalled

When 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.

A new issue has been created to handle refactoring the module-exists checks into local variables for readability. See related issue.