The recently enabled modules submit callback is not building up the list correctly and throwing notices for every module.

Proposed Solution

Split the traversing through the form to collect the default and the comparison operation into separate loops.

CommentFileSizeAuthor
#16 module_filter-undefined_index_recent_modules_submit-2857431-16.patch1.24 KBgreenskin
#15 2857431-module_filter-undefined-index-notice-15.patch802 bytessmustgrave
#9 module_filter_module_-_D8_-____Sites_d8_public_.png181.66 KBjoelpittet
#9 module_filter_module_-_D8_-____Sites_d8_public_.png104.9 KBjoelpittet
#2 2857431-2.patch1.16 KBjoelpittet

Comments

joelpittet created an issue. See original summary.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Active » Needs review
StatusFileSize
new 2857431-2.patch1.16 KB
greenskin’s picture

greenskin commented
Status: Needs review » Postponed (maintainer needs more info)

What is the notice being thrown, and what version of Drupal are you running? I'm not able to reproduce the issue. Also, what version of PHP?

I'd really rather not split up into separate loops for a couple different reasons. You would essentially be looping over the exact same array twice unnecessarily, and adding another array to memory (oh, and more lines of code). The more modules in the list, the more that extra loop and array costs. Looking at the submitted patch, I'm not seeing how the extra loop would really make any difference. It's essentially doing the same thing as the single loop except with the additional array to memory and going through the loop a second time.

greenskin’s picture

greenskin commented

Actually, as I've been looking more at the patch, I see the second loop is missing it's outer loop on packages. So that loop's $module is actually the package name and $details is the array of modules in that package.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Postponed (maintainer needs more info) » Needs review

Version of PHP is 5.6, but should show those notices if you have the error level turned up/E_ALL.

You may be able to do it in one loop but being nested as it is with packages in the form and by modules in the form state values means it will be a pain and less clear. Feel free to try though.

greenskin’s picture

greenskin commented
Status: Needs review » Postponed (maintainer needs more info)

If it's a matter of clarity versus performance and efficiency, adding a comment will be sufficient to clarify here. I'd also suspect if the original code is giving you notices then a fixed version of your patch (see comment #4) will also (though I can't be sure without knowing the notice message) given it's essentially identical except with building a defaults array rather than checking the source directly. When looping over the form state it's first on packages then on modules, so nested loops is necessary. The patch would do this nested loop twice, so if it's a pain and less clear then it's doubled down by needing to do it twice.

Yep, my error level is configured to E_ALL, however I'm running PHP 7.0 so I don't know if that's making the difference. Can you copy and paste in one of the notices, and what is the version of Drupal you are running? I've tested with the latest stable release 8.2.7.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Postponed (maintainer needs more info) » Needs review

I'm using 8.4.x branch, just tested it out in 8.3.x branch of core and it's the same. Here's the error message when enabling a new module, there is one of these notices (with xdebug stack trace) for every module on the page.

I'll leave it to you to clearly rewrite the code I wrote into one loop as you seem know how to do that.

Notice: Undefined index: serialization in module_filter_system_modules_recent_enabled_submit() (line 160 of modules/module_filter/module_filter.module).

module_filter_system_modules_recent_enabled_submit(Array, Object)
call_user_func_array('module_filter_system_modules_recent_enabled_submit', Array) (Line: 111)
Drupal\Core\Form\FormSubmitter->executeSubmitHandlers(Array, Object) (Line: 51)
Drupal\Core\Form\FormSubmitter->doSubmitForm(Array, Object) (Line: 585)
Drupal\Core\Form\FormBuilder->processForm('system_modules', Array, Object) (Line: 314)
Drupal\Core\Form\FormBuilder->buildForm(Object, Object) (Line: 74)
Drupal\Core\Controller\FormController->getContentResult(Object, Object)
call_user_func_array(Array, Array) (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 574)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 124)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}()
call_user_func_array(Object, Array) (Line: 144)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 64)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 57)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 99)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 78)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 50)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel->handle(Object, 1, 1) (Line: 656)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)
joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

I can't reproduce it in 8.2.x so this is really just a bug in> 8.3.x, sorry didn't think that array structure would have changed between versions. I'll try to track down the patch that made this change in core.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
StatusFileSize
new module_filter_module_-_D8_-____Sites_d8_public_.png104.9 KB
new module_filter_module_-_D8_-____Sites_d8_public_.png181.66 KB

Here's a couple screenshots you can see the structure of $form_state->getValue('modules') has changed.

greenskin’s picture

greenskin commented
Assigned: Unassigned » greenskin

Ah, okay. That then makes sense why I wasn't seeing it. I'll see what I can do which will work with the current stable Drupal release and the 8.3.x branch. Thanks joelpittet!

kingdutch’s picture

kingdutch
he/him
Primary language English
commented

Related change notice is here: https://www.drupal.org/node/2851653
This makes the module filter module unusable on 8.3.0 and up because enabling modules is now impossible.

kingdutch’s picture

kingdutch
he/him
Primary language English
commented
Status: Needs review » Needs work

Applied the patch and went to the modules page. Filtered on the migrate module and selected it to enable. After submitting the form the module was not enabled.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Needs work » Needs review

@Kingdutch, that sounds unrelated this issue is about "Recently Enabled" notices, it's much less important than what you are mentioning but likely related tangentially.

jaydee1818’s picture

jaydee1818 commented

I'm having the same issue:

Drupal 8.3.2
PHP 7.0.14

smustgrave’s picture

smustgrave commented
StatusFileSize
new 2857431-module_filter-undefined-index-notice-15.patch802 bytes

I'm using Drupal 8.3.3 and PHP 5.5.38. I was also seeing the notice. I just started looking into Drupal 8 so apologizes.

greenskin’s picture

greenskin commented
StatusFileSize
new module_filter-undefined_index_recent_modules_submit-2857431-16.patch1.24 KB

This patch should fix the issue and remains backward compatible with Drupal 8 versions prior to 8.3.0.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

That seems to avoid errors, but currently the filtering doesn't seem to be working, so can't RTBC this quite yet to test it properly.

OnkelTem’s picture

OnkelTem commented
Priority: Normal » Major

Yes, I confirm that while warnings disappear after applying #16 modules can not be installed if you locate a module using the filter.

What I mean is:

1) If I type e.g. "SEO" and the list is filtered to present a list of matching modules, and there I tick a module and press the Install button, it doesn't get installed.
2) If I don't type anything in the filter and scroll to the same module, then tick it and press the Install button, it does get installed.

I set it to major because it's the primary function of the module: to filter and install.

apratt’s picture

apratt commented

Seeing the same behaviour with drupal 8.3.5 on PHP 7.1

mengi’s picture

mengi commented

Confirm on drupal 8.3.7 and php 5.6. Just like #18 says, searching for a module doesn't allow it to be enabled.

ccjjmartin’s picture

ccjjmartin commented
Status: Needs review » Needs work

Updating status based on the last 4 comments to "needs work".

greenskin’s picture

greenskin commented
Status: Needs work » Needs review

I just tested with a fresh download of Drupal version 8.3.7, running PHP 5.6.31. The only non-core modules included were devel and module_filter. I have not been able to replicate the issue of a module not installing when the list is filtered. I've even tried filtering, checking a module to enable, changed the filter so that module is hidden, and submit. The module installs successfully.

The changes provided by the patch run whether the list is filtered or not (the filtering is purely JavaScript and affects nothing server side). Try clearing cache and if the problem persists please open a separate ticket.

websiteworkspace’s picture

websiteworkspace commented

What is the projected date for the fix for this problem to be integrated and released as a new version?

mengi’s picture

mengi commented

I tried with fresh install, #16 works fine. No warnings and I am able to filter and enable a module.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Needs review » Reviewed & tested by the community

Same

  • greenSkin committed 1a3e58b on 8.x-3.x 
    Issue #2857431 by joelpittet, greenSkin, smustgrave:...
alphawebgroup’s picture

alphawebgroup
he/him
Primary language English
Location Kyiv / Toronto
commented
Status: Reviewed & tested by the community » Fixed

looks like it's fixed now

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.