Warning in features_help() if README.txt was deleted or made unreadable on deployment

I agree - we have a deploy script that strips README.txt files out of production site builds so there ends up being a few stray warnings.

In this patch, I've copied some text from the Features project page as an alternative when the README file doesn't exist. I've also added some 'Useful Links' although this does run the risk of broken links in the future.

The patch is testing if the README.txt file exists rather than if it is readable which limits the usefulness of this patch.

Attached is a revised patch with the line
if (file_exists($readme)) {
replaced by
if (is_readable($readme)) {
This tests both if the file exists and is it readable, satisfying either condition.

I've successfully verified patch #4 against the latest 7.x-2.x-dev with unreadable and missing README.txt files. Thanks, izmeez.

Some thoughts. You can disagree and convince me otherwise :)

Considering that the README.txt will not be translated, and considering that this is meant as a fallback behavior: Does it really make sense to spam the t() function and add all these strings to the translation system? And does it make sense to translate this piece by piece, or would it be better to translate bigger chunks? How do other modules do this? I am not sure about this myself..

The else { is superfluous, because we have a return ..; in the if (..) [ block.

The string concat, if we do want to do a string concat with t() calls, could be like this instead:

  return '<h3>' . t('About') . '</h3>'
    . '<p>' . t(..) . '</p>
    . ....
  ;

This way we don't have to repeat $output on every line..
On the other hand, doing it like in the patch would allow to add conditional snippets in the future easily.. (not sure if we want that).

The ternary expression at the top could be broken to multiple lines.

return module_exists('markdown')
  ? filter_xss_admin(module_invoke('markdown', 'filter', 'process', 0, -1, $output))
  : '<pre>' . check_plain($output) . '</pre>';

Or replace the ternary with if().

The links could be done with l() + t()?

Just saying:
I agree with the main goal of this issue!
We don't want a broken help page or errors in the log, on sites where the README.txt was removed.

I am also having a look at the main README.txt of the module.
It looks horribly outdated!
In fact the last commit was 2013.

The "fallback" text proposed here is already more suitable as a help text than the README.
However, seeing the sad state of the README.txt, is there any hope that we would keep this help text up to date?

Who is the target audience of this help text? Are people actually reading this? Which answers would people find here which they cannot find elsewhere?

I know this is just the same as before. So changing it might be out of scope. But:

      $readme = drupal_get_path('module', 'features') . '/README.txt';

drupal_get_path() is not needed within the current module. A simple __DIR__ is enough. Or dirname(__FILE__) for ancient PHP versions.

If we use drupal_get_path(), we might want to prepend DRUPAL_ROOT . '/', so we get an absolute path, and php does not need to guess based on the currently registered paths.
__DIR__ already does give us an absolute path.

Of course drupal_get_path() is still needed if:
- we want to access a file in another module.
- we need the relative path (relative to Drupal root), not the absolute filesystem path. E.g. in drupal_add_css().

I notice the original code is completely wrong.

module_invoke('markdown', 'filter', 'process', 0, -1, $output);

In practice this means calling markdown_filter(), after
- including the file that contains the function, if registered via hook_hook_info().
- checking that the function exists.

There are two problems with this:
- hook_filter() is not a hook.
- There is no function markdown_filter() in any 7.x-*.* release of markdown module.

This means module_invoke() will return NULL.
This NULL is then passed to filter_xss_admin().

What a mess.

easy_breadcrumb shows how to do the markdown part.

/**
 * Implements hook_help().
 */
function easy_breadcrumb_help($path, $arg) {
  switch ($path) {
    case 'admin/help#easy_breadcrumb':
      $readme = file_get_contents(dirname(__FILE__) . '/README.md');
      $output = '';

      if (module_exists('markdown')) {
        $filters = module_invoke('markdown', 'filter_info');
        $info = $filters['filter_markdown'];

        if (function_exists($info['process callback'])) {
          $output = $info['process callback']($readme, NULL);
        }
      }

      if ($output === '') {
        $output = '<pre>' . $readme . '</pre>';
      }

      return $output;
  }
}

It does NOT check if the README.md exists.. but it does invoke the markdown filter in the correct way, I suppose.

It could have called _filter_markdown() directly, but I suppose this is considered a private function which might change in future versions.

Given that this is currently defunct:
What about, as a first step, we disable the features_help(), let it return NULL?

This way we preserve existing behavior (it is already not working), but prevent error messages.

We create a follow-up ticket and reference it in a @todo comment.

Given that this is currently defunct:

Well.. it actually works if markdown module is not enabled!
So a lot of my previous comments become obsolete :)

And see there, there is already an issue and a patch: #1941894: Help is missing if Markdown is enabled.

Changing title and summary so that people might find this more easily when searching the warning text from watchdog.

I call a race between this issue and #1941894: Help is missing if Markdown is enabled.
Whichever gets rtbc'd first, the other will need a reroll.

@justinstandring
If we still want to have a "fallback help text" in case the README was removed, let's do this in a follow-up issue.

This was the wrong patch. Silly me.

I keep patch #4 enabled as an alternative.

The patch in #17 looks fine and also applies without difficulty to the latest features-7.x-2.x-dev 2019-10-20 I am marking it as RTBC.
The race is on.

Thanks for reviewing, @izmeez!
The other issue still has ongoing discussion. So this one has won the race!

I am going to let this ferment for a few days or more, before I commit.

Setting the issue credit. I was unaware how this works when I did the commit.

Thanks to everyone for patches and comments, and jbrauer for the original problem description!

EDIT: Not sure if this actually worked. Will have to look into this later.