Help is missing if Markdown is enabled

I confirm that this is still the case.

The patch looks good, but perhaps some changes are possible:
- Early return instead of if / else?
- Check if README file exists and is readable. See #2473935: Warning in features_help() if README.txt was deleted or made unreadable on deployment. If not, return NULL.

I am going to create another version of the patch.

Btw, I like that the functionality is split into a separate function!

Check if README file exists and is readable.

I leave this part to #2473935: Warning in features_help() if README.txt was deleted or made unreadable on deployment so that we have something to do there.

I believe we can go a bit easier (i.e. less defensive) on the helper:

1. +++ b/features.module
   @@ -366,14 +366,65 @@ function features_permission() {
   +  if (!module_exists('markdown')) {
   +    // The markdown module is not installed.
   +    return NULL;
   +  }
   +  /* @see \markdown_filter_info() */
   +  $filters = module_invoke('markdown', 'filter_info');
   

   module_invoke() already calls function_exists(), so the module_exists() call can be skipped.

2. +++ b/features.module
   @@ -366,14 +366,65 @@ function features_permission() {
   +  if (!isset($filters['filter_markdown']['process callback'])) {
   +    // The version of the markdown module does not define a filter named
   +    // 'filter_markdown'.
   +    return NULL;
   +  }
   

   Both 7.x branches of markdown define the "filter_markdown" filter and have done so since the initial port. This should never happen, and the check would likely mask other errors.

3. +++ b/features.module
   @@ -366,14 +366,65 @@ function features_permission() {
   +  $info = $filters['filter_markdown'];
   +  $function = $info['process callback'];
   +  if (!is_callable($function)) {
   +    // The 'filter_markdown' filter has a defunct filter callback.
   +    return NULL;
   +  }
   

   Combined with the above remarks I'd suggest to switch to drupal_array_get_nested_value() in order to condense the code.

Thanks for the review!

I believe we can go a bit easier (i.e. less defensive) on the helper:

Normally I tell people to be more defensive, not less :)

We can perhaps condense and combine separate conditions, if the intent is still clear.
I wonder what the benefit is of doing that. I think in the end whatever is more clear without being distracting should be the solution.
(any possible performance difference is negligible here)

module_invoke() already calls function_exists(), so the module_exists() call can be skipped.

We would then have to check if the result of module_invoke() might be NULL.

Both 7.x branches of markdown define the "filter_markdown" filter and have done so since the initial port. This should never happen

Except in the new future version, or if there is some custom modification :)
I agree it is highly unlikely.

Perhaps we could combine it with the previous condition.
But then it would be less clear that these are distinct cases.

the check would likely mask other errors.

Like which?
The way I see it, the different checks help to distinguish different sources of failure.
I did not add any watchdog() or drupal_set_message(), because these cases are not really considered errors. But some could easily put debug break points on the different returns.

Combined with the above remarks I'd suggest to switch to drupal_array_get_nested_value() in order to condense the code.

So, like this?

  $function = drupal_array_get_nested_value($filters, array('filter_markdown', 'process callback'));

Would it not be more clear like this?

  $function = $filters['filter_markdown']['process callback'];

In general I like to keep things defensive and explicit, even if this adds some verbosity. In most of D7 contrib, the opposite is the case, and it causes suffering..

For this issue I think it is a matter of taste, and not really a blocker. There won't be a regression caused by overly defensive code.

Normally I tell people to be more defensive, not less :)

With you on that, but too much verbosity creates a maintenance burden. :)

For this issue I think it is a matter of taste, and not really a blocker. There won't be a regression caused by overly defensive code.

Agreed, that's why I didn't set it to "Needs work".

I just had another look at check_markup(), and I now do believe that (ab-)using the filter hook (or API, via filter_get_filters()) is the wrong approach, because then we might as well cover the whole API surface (calling "prepare callback" if given, passing all arguments).

The best variant might be to just check for _markdown_tips() and call that directly. Ultimately though it's your call.

With you on that, but too much verbosity creates a maintenance burden. :)

I don't think so. All this stuff is tucked away in a single function which does only one thing.

I just had another look at check_markup(), and I now do believe that (ab-)using the filter hook (or API, via filter_get_filters()) is the wrong approach, because then we might as well cover the whole API surface (calling "prepare callback" if given, passing all arguments).

You have a point there.
The proposed solution has precedent in easy_breadcrumb_help(), see also my comment in #2473935-11: Warning in features_help() if README.txt was deleted or made unreadable on deployment, and the issue in easy_breadcrumb #2928364: Implement hook_help D7.
It is not perfect, but we may have to live with that.

To use the API in the correct way, we would have to replicate the logic from check_markup().
This means we would also have to craft placeholder values for additional parameters to those functions.
In theory, a future version of markdown module might use more of the available parameters, causing problems for us.
Each of the extra parameters would need to conform to the expected format. With how Drupal works, we could put arbitrary effort into this..

The best variant might be to just check for _markdown_tips() and call that directly. Ultimately though it's your call.

I don't see any _markdown_tips(). There is _filter_markdown_tips(), but it clearly does not do what we need.
Perhaps you mean _filter_markdown().
Or we could call \Michelf\MarkdownExtra::defaultTransform() directly.

The problem is all of these are "internal implementation", and could, in theory, change in a future version of the module. E.g. if they choose CommonMark over MarkdownExtra.
Perhaps not very likely at this stage of Drupal 7, but I think it makes sense to attempt to use only the public API of other modules.

In summary:
None of the functionality of markdown module is designed or advertised as "public API" to use in code. We are left to choose the lesser evil.

I'd say we should stick with the current approach until a really superior alternative comes up. This could be done as follow-up.

I am going to leave this open for a bit, perhaps someone has a better idea and can propose that as a competing patch.

I don't see any _markdown_tips(). There is _filter_markdown_tips(), but it clearly does not do what we need.
Perhaps you mean _filter_markdown().

Yup, that was likely a brainfart, sorry about that. My intention was that with function_exists('_filter_markdown') the implementation could be reduced to the bare minimum necessary for markdown conversion. But I'm already bikeshedding this issue for way too long, so I'm cool with whatever solution you pick.

Reroll of the patch in #4, and new patch which attempts to fully simulate check_markup().
This should be more robust for future versions of markdown module.

@donquixote I can confirm both the re-roll and new patch in #11 apply without difficulty to the current 7.x-2.x-dev (2019-11-02). I have looked at the patches and the interdiff which is helpful, thank you. Unfortunately, I do not know the code well and cannot comment on your conclusions but "more robust" sounds good.

Thanks for checking @izmeez!
I will keep this open for a bit longer to perhaps get other opinions.
The second patch feels like overkill, but in the end it might still be the right thing to do.

The thing to be verified by a 2nd person is that the additional complexity I added does not contain mistakes or have unintended effects I overlooked.