The "missing/moved modules" message (see https://www.drupal.org/node/2487215) added in Drupal 7.50 is maybe too harsh as a PHP warning. I think it should be a PHP notice instead.

This message occurs when Drupal is trying to load a file but doesn't find it, or doesn't find it in the expected place. To me this is similar to code that tries to access an array key that doesn't exist, for which PHP throws a notice, not a warning.

This change would help the message be less visible in places where it is currently unwanted - for example, I believe Drush would stop displaying it except when --verbose is used, and it would also not display on sites (e.g. staging sites) that are configured to display errors and warnings to the screen but not notices. However, it would still display on development sites that are configured to display all messages to the screen, which would help developers find mistakes in their code (I think that was the main motivation for introducing this message in the first place).

In https://www.drupal.org/node/2487215#comment-11539563 @hstrindb suggested that it should still be a PHP warning if it is an enabled module that has gone missing, but a notice otherwise. I agree in theory, but I think it would be a pain to code that - so I'm not including that in the patch for now. If an enabled module has gone missing, this will still trigger a notice on basically every page request (so it will fill up the watchdog logs much more than a non-enabled module that is missing) and likely something else noticeable will be wrong with your site too. So I don't think it has to be a PHP warning in that case if we can't easily preserve that.

Comments

David_Rothstein created an issue. See original summary.

David_Rothstein’s picture

Status: Active » Needs review
FileSize
3.59 KB

Here is a patch.

       // If error type is 'User notice' then treat it as debug information
-      // instead of an error message, see dd().
-      if ($error['%type'] == 'User notice') {
+      // instead of an error message, unless we know it came from somewhere
+      // other than debug().
+      if ($error['%type'] == 'User notice' && !drupal_static('_drupal_trigger_error_with_delayed_logging')) {
         $error['%type'] = 'Debug';
         $class = 'status';
       }

This workaround is necessary since #296574: Provide debug facility for general and testing forces all E_USER_NOTICE messages to be treated like debug messages. I'll post a separate issue to fix that problem for real (Drupal 8 has it too).

Status: Needs review » Needs work

The last submitted patch, 2: missing-modules-notice-2844716-2.patch, failed testing.

David_Rothstein’s picture

Status: Needs work » Needs review
FileSize
5.05 KB

Updating the tests.

David_Rothstein’s picture

Updating some additional code comments/etc. also.

David_Rothstein’s picture

This workaround is necessary since #296574: Provide debug facility for general and testing forces all E_USER_NOTICE messages to be treated like debug messages. I'll post a separate issue to fix that problem for real (Drupal 8 has it too).

Now posted.

David_Rothstein’s picture

This change would help the message be less visible in places where it is currently unwanted - for example.... it would also not display on sites (e.g. staging sites) that are configured to display errors and warnings to the screen but not notices.

Currently that doesn't work actually - even as a notice, it will still display to the screen on those sites.

This is another independent bug that affects Drupal 8 too. I created the linked issue with a patch to fix it.

Vinoth Govindarajan’s picture

The patch works as expected, thanks @David_Rothstein

Neograph734’s picture

Status: Needs review » Reviewed & tested by the community

I second that. Provided patch makes drush behave much better, especially when downloading and enabling a new module in a project that contains sub-modules.

Setting this to RTBC as it solves the current problem, but feel free to set it back if the other issues have to be implemented first.

stefan.r’s picture

Well the goal was to make the error annoying (which included showing up in Drush) so developers would actually fix the issue, and assuming that error_level would be set to 0 on production sites.

I can see wanting to hide it on staging sites (or sites with "Errors and warnings only"), but not sure about hiding it in Drush -- what about existing sites that are not under active development anymore but still get the occasional drush command from non-developers/site admins, wouldn't we want to notify them about needing to fix the issue?

Neograph734’s picture

@stefan.r you have a point there, but the current situation is over enthusiastic, also throwing the error (for every sub-module) when installing a module that contains sub-modules and breaking some big installation profiles because of `module_hook` invoking disabled modules #1948702: module_hook is broken for disabled modules and included hooks (Both mentioned in #2773959-39: "The following [module/theme] has moved within the file system:" error after downloading new module or theme). So it now leading towards a point where people might start ignoring it because it is everywhere.

It might be that after those issues are committed everything gets a little more quiet and the error is only shown during some drush tasks (which I personally can live with).

UPDATE
Oh, in fact it appears this issue solves the issue with the module installation (see my comment above).

amit0212’s picture

patch

Patch attached.

Created against 7.x.

Tested against project/panopoly latest version with:
- search_api and features enabled (to trigger specific step mentioned above)
- several modules in the profile/modules directly not installed

Again - not sure if this is the best method to tackle this problem, but hoping to get conversation going.