Patch_status with drupal 7.50 shows disabled modules as missing

Just changing the title for clarity.

Just to check, you said disabling patch_status meant the warnings went away. Do they come back if you re-enable patch_status?

Yes. That is correct.

This can be reproduced on a fresh Drupal 7 install by having a contrib module present that is not enabled or installed (e.g. advanced_help) along with patch_status enabled. Leave admin/config/development/logging displaying all messages (to screen). Ensure admin/reports/updates/settings "Check for updates of disabled and uninstalled modules and themes" is checked. Then view available updates report, admin/reports/updates.

Oddly, this not only gives warning about module-disabled but also core missing which of course it is not:

User warning: The following module-disabled is missing from the file system: advanced_help. In order to fix this, put the module-disabled back in its original location. For more information, see the documentation page. in _drupal_trigger_error_with_delayed_logging() (line 1128 of /.../includes/bootstrap.inc).
User warning: The following core is missing from the file system: drupal. In order to fix this, put the core back in its original location. For more information, see the documentation page. in _drupal_trigger_error_with_delayed_logging() (line 1128 of /.../includes/bootstrap.inc).

Disabling patch_status makes the warnings go away even if it is not uninstalled.

And flushing caches and re-enabling patch_status causes the the warnings to return.

Hope this helps track down the problem.

I noticed a different module with a similar issue and a patch for where module_load_include is used, #2782135-5: Incorrect module load causes missing module user warning with 7.50

@WorldFallz The patch in #9 applies without problems and resolves the missing module warning for disabled and uninstalled modules, but it does not appear to be showing the name of the patch file even for the patch_status module where the new *.patch file is.

I haven't done any more extensive testing but I think it needs more work.

Sorry, just changing status.

This is now working for contrib module directories that contain *.patch file(s). In my testing the core directory did not have any *.patch files.

There is another issue where drush make is used and the patches are listed in PATCHES.txt file, #2664996: PATCHES.txt should not need to be world readable which does not behave the same.

The patch in #13 solves the issue of modules showing as missing when they are extra modules that are not enabled, or have been disabled and uninstalled but still present in the modules directory.

I am tempted to consider this major since this issue arose with the changes to drupal 7.50 core and can create a number of entries in the watchdog log.

Tested the new release and it does not solve the problem in the issue opening description, disabled modules showing as missing.

The oddity described in comment #6 "... The following core is missing ..." appears to be resolved.

@rupertj Sorry, I am not going to repeat myself with a new thread.

I do not know how the issue can be expressed more clearly than it has. I have answered questions for more information. Comment #6 described how to reproduce the issue on a fresh install. I have reviewed and tested patches.

The "fix" was not provided as a patch for testing and review. Maybe the "fix" should explain what it was intended to fix. I think this issue has been hijacked by a fix that does not address the issue. If the "fix" fixes part of the problem fair enough, but it still needs work to fix the main problem, disabled modules trigger missing module warning!

I have changed the status back to "Needs work".

Sad to see a useful module hit the rocks like this.

@Fabianx Thanks for your added comments.

I am not sure the patch_status for disabled modules is needed. In case it is, the solution in the patches expressed above would also solve the problem.

I think it is helpful to show patch_status for disabled modules to clearly show their status and to keep consistency whether the module is enabled or disabled.

Is it this part of the new commit that needs further work?

@@ -467,3 +467,22 @@ function patch_status_patch_name_from_url($patch_url) {
 
   return drupal_strip_dangerous_protocols($patch_url);
 }
+
+/**
+ * Returns the path to a patch on the local filesystem, so it can be linked to.
+ * @param $project_type
+ * @param $project_name
+ * @param $patch_filename
+ * @return string
+ */
+function patch_status_get_path($project_type, $project_name, $patch_filename) {
+
+  if ($project_type == 'core') {
+    $project_path = DRUPAL_ROOT;
+  }
+  else {
+    $project_path = drupal_get_path($project_type, $project_name);
+  }
+
+  return $project_path . '/' . $patch_filename;
+}

I've added the 2nd part of the patch in #13 to the current dev.

@@ -398,7 +398,7 @@ function patch_status_get_patches($project_type, $project_name) {
     $options = array('recurse' => FALSE);
   }
   else {
-    $project_path = drupal_get_path($project_type, $project_name);
+    $project_path = dirname(drupal_get_filename($project_type, $project_name, NULL, FALSE));
     $options = array();
   }

This fixes the issue.

The fix that was committed is not as succinct as the patch offered in #13 while it adds another function that may add readability but does not incorporate the full change in the first part of #13.

I would suggest an alternative for the commit,

@@ -289,8 +289,8 @@ function theme_patch_status_update_report(array $variables) {
           $patch_link = l($patch_link_name, $patch, array('external' => TRUE));
         }
         else {
-          $patch_link_name = drupal_get_path($project['project_type'], $project['name']) . '/' . $patch;
-          $patch_link = l($patch, $patch_link_name);
+          $patch_link_path = dirname(drupal_get_filename($project['project_type'], $project['name'], NULL, FALSE)) . '/' . $patch;
+          $patch_link = l($patch, $patch_link_path);
         }

@@ -398,7 +398,7 @@ function patch_status_get_patches($project_type, $project_name) {
     $options = array('recurse' => FALSE);
   }
   else {
-    $project_path = drupal_get_path($project_type, $project_name);
+    $project_path = dirname(drupal_get_filename($project_type, $project_name, NULL, FALSE));
     $options = array();
   }

This is essentially the same as the patch in #13 with the variable name change from $patch_link_name to $patch_link_path.

If it is decided to add the new function patch_status_get_path the following lines need review,

  else {
    $project_path = drupal_get_path($project_type, $project_name);
  }

@Fabianx I understand the concept you present but can you identify how it should be implemented? Thanks.

Just adding the reference to the pertinent change record, Performance improvements for drupal_get_filename() https://www.drupal.org/node/2581445