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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

RajabNatshah created an issue. See original summary.

Rajab Natshah’s picture

Title: Notice: Undefined index: total When we enable the module in the installation » Notice: Undefined index: total When we enable the module in the installation step
Rajab Natshah’s picture

Rajab Natshah’s picture

Status: Needs work » Needs review
drunken monkey’s picture

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

What 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 outer if. (Or the else should be an elseif (!$success).)

Rajab Natshah’s picture

Issue summary: View changes
Rajab Natshah’s picture

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

Hi Thomas,

I have updated the summery for this issue.
Please have a look.

Rajab Natshah’s picture

Issue summary: View changes
Rajab Natshah’s picture

Issue summary: View changes
Rajab Natshah’s picture

drunken monkey’s picture

Component: General code » Framework
FileSize
539 bytes

OK, 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, unless processBatch() 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.

Rajab Natshah’s picture

Thank 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.

 if (defined('MAINTENANCE_MODE') && MAINTENANCE_MODE != 'install' &&  MAINTENANCE_MODE != 'update') {

 }

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.

drunken monkey’s picture

Hm, 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?

drunken monkey’s picture

Does the patch in #11 work for you now?

Rajab Natshah’s picture

I will switch to use #13

nedjo’s picture

I'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:

  • Install Drutopia. See the Composer install-project command here.
  • Do a manual install. At the Configure Site install stage, select at least the Drutopia Page and Drutopia Search features to enable. The install error message does not show up during install, but rather on the first post-install page, when the installer has forwarded to the newly-installed site's home page.

Unfortunately it's a slow install, so time consuming to test with.

drunken monkey’s picture

Thanks, 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:

Fatal error:  Declaration of Drupal\comment\Plugin\Menu\LocalTask\UnapprovedComments::getTitle() must be compatible with Drupal\Core\Menu\LocalTaskDefault::getTitle(?Symfony\Component\HttpFoundation\Request $request = NULL)

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?

nedjo’s picture

@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:

  • Install Drutopia. See the Composer install-project command here.
  • Replace the patched version of /modules/contrib/search_api with the current, unpatched release.
  • Do a manual install. At the Configure Site install stage, select at least the Drutopia Page and Drutopia Search features to enable. The install error message does not show up during install, but rather on the first post-install page, when the installer has forwarded to the newly-installed site's home page.

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.

drunken monkey’s picture

Yes, 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!

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.

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?

Rajab Natshah’s picture

Using #13 with Search API 8.1.8

drunken monkey’s picture

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

Does 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:

The website encountered an unexpected error. Please try again later.
Drupal\Core\Config\UnmetDependenciesException: Configuration objects provided by block.block.octavia_footer_menu (block), block.block.seven_help (help, block) in Drupal\Core\Config\UnmetDependenciesException::create() (line 98 of core/lib/Drupal/Core/Config/UnmetDependenciesException.php).

Rajab Natshah’s picture

Issue summary: View changes
Rajab Natshah’s picture

Yes, only in the step of enabling the module in installation profiles

Rajab Natshah’s picture

After search_api 8.x-1.12 we can install or update without issues.

Rajab Natshah’s picture

Status: Postponed (maintainer needs more info) » Reviewed & tested by the community

Still I think it's better to have the following:


if (defined('MAINTENANCE_MODE') && MAINTENANCE_MODE != 'install' &&  MAINTENANCE_MODE != 'update') {

}

As they do it in most of Drupal core bulk or batch jobs.

drunken monkey’s picture

Status: Reviewed & tested by the community » Closed (outdated)

After search_api 8.x-1.12 we can install or update without issues.

OK, 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.

tobiasb’s picture

Status: Closed (outdated) » Needs work

I got the same, I believe because of this code in hook_install:

  \Drupal::moduleHandler()->loadInclude('locale', 'translation.inc');
  \Drupal::moduleHandler()->loadInclude('locale', 'batch.inc');

  $langcode = \Drupal::languageManager()->getCurrentLanguage()->getId();
  $options = _locale_translation_default_update_options();
  $options['customized'] = TRUE;
  $options['overwrite_options']['customized'] = TRUE;
  $operations[] = ['locale_translation_batch_fetch_import', ['degov_auto_crop', $langcode, $options]];
  $batch = array(
    'title' => t('Importing translations.'),
    'operations' => $operations,
    'error_message' => t('An error occured during deGov - Automatic cropping (degov_auto_crop) module installation.'),
  );
  batch_set($batch);

btw: This is not required when you use hook_locale_translation_projects_alter. Drupal imports the translations on a new installation.

Rajab Natshah’s picture

Correction for the logic


  if (!defined('MAINTENANCE_MODE')) {

  }

tobiasb’s picture

My solution was to set

\Drupal::state()->set('search_api_use_tracking_batch', FALSE);

and at the end \Drupal::state()->set('search_api_use_tracking_batch', TRUE);

Peter Majmesku’s picture

Just do not check for not existing array key.

drunken monkey’s picture

@ 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.)

borisson_’s picture

Status: Needs work » Reviewed & tested by the community

This #13 patch shouldn't hurt, so I think there's nothing wrong with committing that.

drunken monkey’s picture

Status: Reviewed & tested by the community » Fixed

Alright, thanks for weighing in!
Committed.

  • drunken monkey committed 1ace77e on 8.x-1.x
    Issue #2931562 by drunken monkey, RajabNatshah, nedjo, borisson_: Fixed...

Status: Fixed » Closed (fixed)

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