Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
Given that we do have "Drupal" version "~8.6.0" files ready to install
And we added "search_api" module in our "[custom profile].info.yml" to be enabled at the install step
And we do have some [Default content] nodes ..
Then we will face this error report at the install
Notice: Undefined index: total in Drupal\search_api\Task\TaskManager->finishBatch() (line 334 of modules/contrib/search_api/src/Task/TaskManager.php).
Drupal\search_api\Task\TaskManager->finishBatch(1, Array, Array, Object) (Line: 456)
_batch_finished() (Line: 98)
_batch_page(Object) (Line: 647)
install_run_task(Array, Array) (Line: 555)
install_run_tasks(Array) (Line: 117)
install_drupal(Object) (Line: 44)
Proposed resolution
- Make sure that Total is set before calling it in the array.
- Not to do any type of indexing while the Drupal site MAINTENANCE_MODE is 'install' or 'update'
Remaining tasks
User interface changes
API changes
Check for MAINTENANCE_MODE != 'install' and MAINTENANCE_MODE != 'update' before working on any tasks or events.
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#30 | 2931562-results-php-notice.patch | 536 bytes | Peter Majmesku |
| |||
#13 | 2931562-13--undefined_total_in_task_manager_batch_finish.patch | 1.08 KB | drunken monkey |
| |||
#11 | 2931562-11--undefined_total_in_task_manager_batch_finish.patch | 539 bytes | drunken monkey |
|
Comments
Comment #2
Rajab Natshah CreditAttribution: Rajab Natshah at Vardot commentedComment #3
Rajab Natshah CreditAttribution: Rajab Natshah at Vardot commentedComment #4
Rajab Natshah CreditAttribution: Rajab Natshah at Vardot commentedComment #5
drunken monkeyWhat do you mean with “enable the module in the installation step”? How can I reproduce this issue?
From looking at the code, I don't really see how
$results['total']
could ever be unset.(Just want to make sure there's no underlying problem which we're masking, instead of fixing, with this patch.)
Also, I don't think we want to display an error message in this case? So there should be a nested
if
block, not an additional check in the outerif
. (Or theelse
should be anelseif (!$success)
.)Comment #6
Rajab Natshah CreditAttribution: Rajab Natshah at Vardot commentedComment #7
Rajab Natshah CreditAttribution: Rajab Natshah at Vardot commentedHi Thomas,
I have updated the summery for this issue.
Please have a look.
Comment #8
Rajab Natshah CreditAttribution: Rajab Natshah at Vardot commentedComment #9
Rajab Natshah CreditAttribution: Rajab Natshah at Vardot commentedComment #10
Rajab Natshah CreditAttribution: Rajab Natshah at Vardot commentedNot sure, I'm still testing the fix at:
#2931674: Added the 2931562-3.patch for [Search API] module to fixe the issue of Notice: Undefined index: total When we enable the module in the installation step
Thomas, I think we should add all your notes in the logic.
Comment #11
drunken monkeyOK, thanks for clarifying!
That is unfortunately a bit cumbersome for testing. But could you maybe try out whether the attached patch fixes the problem, too? I guess if someone, for any reason, would set
$context['sandbox']['task_ids']
before we get it, we'd leave actually would leave$context['results']['total']
unset – the only possible way I see, unlessprocessBatch()
is just not called at all (which, I'm pretty sure, would be a bug in Core).Re "Not to do any type of indexing": it's not indexing, it's execution of pending tasks. While not doing that during installation might also be an option, it would be better if we could avoid that. a) because it adds an additional special case we have to keep in mind, and b) because this will leave people with an "unfinished" installation by default, which will cause confusion. (Though there shouldn't be any major problems.)
Also: How do you add default content during the installation? With a
hook_install()
in one of the earlier-installed modules? Is it really necessary to have such content present – it seems to me like whether content is present shouldn't make any difference.Comment #12
Rajab Natshah CreditAttribution: Rajab Natshah at Vardot commentedThank you Thomas!
I will switch to use your patch at this time.
we are using the default content module. which we do have number of nodes and their content. our working examples: Varbase, and Vardoc.
I do think as some Drupal core and contrib modules do a check for MAINTENANCE_MODE != 'install' and MAINTENANCE_MODE != 'update' before working on any tasks or events.
We may need to add this before, but still not sure if that will slow the process if we do have 500k or 1m nodes in the site.
Comment #13
drunken monkeyHm, might also be an idea, right. Not completely sure, though – any other opinions pro/against? The attached patch would implement this.
If there are no further problems with adding a batch during install/update, I'm not sure whether this is actually needed, or even a good idea.
Does the previous patch now work for you?
Comment #14
drunken monkeyDoes the patch in #11 work for you now?
Comment #15
Rajab Natshah CreditAttribution: Rajab Natshah at Vardot commentedI will switch to use #13
Comment #16
nedjoI'm getting this error in the current release of the Drutopia distribution, which matches the user story here (we also have default content installed during the site install process).
The patch in #13 fixed the issue for me.
I also tried the patch from #11 but got no difference--no new error, but the existing error remained.
To reproduce:
Unfortunately it's a slow install, so time consuming to test with.
Comment #17
drunken monkeyThanks, nedjo, that looks like a very good and reliable way to reproduce and debug this, exactly what I need!
However, it seems that distribution has more pressing problems than a notice in this module:
For me, the installation fails with the above error. It seems it doesn't actually work with a current version of PHP at the moment?
Once that's fixed, I'll gladly give it another try. (I already have the debugging set up, so don't want that work to be a waste.)
Also, #11 not working would really point to the Batch API being at fault, apparently not calling the process callback at all but jumping straight to the finish callback. Maybe it gets confused by multiple batches trying to run at once?
Comment #18
nedjo@drunken monkey
I've now added the patch from this issue to the Drutopia distribution, so the testing needs an extra step. I think the following should work:
/modules/contrib/search_api
with the current, unpatched release.Re the error you're getting, it seems core 8.4.x is incompatible with PHP 7.2, see #2932574: Indicate Drupal 8.4.x is not compatible with PHP7.2.
Comment #19
drunken monkeyYes, I happened to notice the message about the patch being apllied, so I've already reverted it locally. Thanks for still pointing it out, though!
OK, then I guess testing this will have to wait until you update your distribution to use Drupal 8.5. Would be great if you could ping me at that point, so I don't forget. (Or, of course, if someone else has similarly simple reproduction steps for me.)
Or should the distribution still work when updating it to use 8.5 right now? If so, how would I do that?
Comment #20
Rajab Natshah CreditAttribution: Rajab Natshah at Vardot commentedUsing #13 with Search API 8.1.8
Comment #21
drunken monkeyDoes this issue still occur for you?
I now tried to test with the latest Drutopia release, which seems to be compatible with PHP 7.3 now, but I get another error during installation:
Comment #22
Rajab Natshah CreditAttribution: Rajab Natshah at Vardot commentedComment #23
Rajab Natshah CreditAttribution: Rajab Natshah at Vardot commentedYes, only in the step of enabling the module in installation profiles
Comment #24
Rajab Natshah CreditAttribution: Rajab Natshah at Vardot commentedAfter search_api 8.x-1.12 we can install or update without issues.
Comment #25
Rajab Natshah CreditAttribution: Rajab Natshah at Vardot commentedStill I think it's better to have the following:
As they do it in most of Drupal core bulk or batch jobs.
Comment #26
drunken monkeyOK, so it seems this is fixed? Then I wouldn’t just commit the patch for no reason, really. As said, this will lead to minor issues every time a search index is part of a module config, so I’d really like to avoid this.
I still couldn’t reproduce this, there’s probably lots of people installing installation profiles with search indexes without any problems, and now Drutopia doesn’t download at all so I still have no chance of testing this.
Comment #27
tobiasbI got the same, I believe because of this code in hook_install:
btw: This is not required when you use hook_locale_translation_projects_alter. Drupal imports the translations on a new installation.
Comment #28
Rajab Natshah CreditAttribution: Rajab Natshah at Vardot commentedCorrection for the logic
Comment #29
tobiasbMy solution was to set
and at the end
\Drupal::state()->set('search_api_use_tracking_batch', TRUE);
Comment #30
Peter MajmeskuJust do not check for not existing array key.
Comment #31
drunken monkey@ Peter: That’s exactly the same patch as in #3. Please see the rest of the discussion and weigh in on the other options.
Especially: Does the patch in #13 work for you? (Same for tobiasb.)
Comment #32
borisson_This #13 patch shouldn't hurt, so I think there's nothing wrong with committing that.
Comment #33
drunken monkeyAlright, thanks for weighing in!
Committed.