backdrop-contrib / search_api

Provides a generic API for modules offering search capabilities
GNU General Public License v2.0
0 stars 5 forks source link

PHP 8.1 Deprecated: Optional parameter $drush declared before required parameter $context #36

Closed yorkshire-pudding closed 1 year ago

yorkshire-pudding commented 1 year ago

On PHP 8.1 site with search_api enabled, this appears at the top of the screen

Deprecated: Optional parameter $drush declared before required parameter $context is implicitly treated as a required parameter in /app/docroot/modules/search_api/search_api.module on line 3401

herbdool commented 1 year ago

This is also an issue for PHP 8.0

herbdool commented 1 year ago

This is in an internal function (with the _ at the front) and only gets called by another internal function using Batch API. Might be enough to just make the parameter required and then add a check for whether it was set or not in the function itself.

function _search_api_batch_indexing_callback(SearchApiIndex $index, $batch_size, $limit, $drush, &$context) {
  // Persistent data among batch runs.
  if (!isset($context['sandbox']['limit'])) {
    $context['sandbox']['limit'] = $limit;
    $context['sandbox']['batch_size'] = $batch_size;
    $context['sandbox']['progress'] = 0;
  }

  // Persistent data for results.
  if (!isset($context['results']['indexed'])) {
    $context['results']['indexed'] = 0;
    $context['results']['not indexed'] = 0;
    $context['results']['drush'] = isset($drush) ? $drush : FALSE;;
  }
earlyburg commented 1 year ago

/app/docroot/ this is not the way

@herbdool - hit me with a pull request and I will test it.

yorkshire-pudding commented 1 year ago

/app/docroot/ this is not the way

? that is just the lando path to the backdrop root that was included in the error message; as there is nothing sensitive in it, I left it in. Not sure what the issue is with including the full path

earlyburg commented 1 year ago

/app/docroot/ this is not the way

? that is just the lando path to the backdrop root that was included in the error message; as there is nothing sensitive in it, I left it in. Not sure what the issue is with including the full path

It was meant as a joke. By the path I know you are running the code in a container and I also know that this CMS uses PHP code that is structured a certain way, and that means one more unstructured variable in a testing equation. Containers do not always function the same way that other environments do.

earlyburg commented 1 year ago

This is in an internal function (with the _ at the front) and only gets called by another internal function using Batch API. Might be enough to just make the parameter required and then add a check for whether it was set or not in the function itself.

function _search_api_batch_indexing_callback(SearchApiIndex $index, $batch_size, $limit, $drush, &$context) {
  // Persistent data among batch runs.
  if (!isset($context['sandbox']['limit'])) {
    $context['sandbox']['limit'] = $limit;
    $context['sandbox']['batch_size'] = $batch_size;
    $context['sandbox']['progress'] = 0;
  }

  // Persistent data for results.
  if (!isset($context['results']['indexed'])) {
    $context['results']['indexed'] = 0;
    $context['results']['not indexed'] = 0;
    $context['results']['drush'] = isset($drush) ? $drush : FALSE;;
  }

I would look for every instance where this hook is called and figure out what the function is being fed. I will have a quick look.

herbdool commented 1 year ago

I already did a text search of the module code. Just the one call. Which makes sense since it's only meant for that one batch.

herbdool commented 1 year ago

Added PR https://github.com/backdrop-contrib/search_api/pull/37

yorkshire-pudding commented 1 year ago

Thanks @herbdool - I've tested this and it fixes the issue. I've had a look at the code and it all makes sense.

yorkshire-pudding commented 1 year ago

@earlyburg - any plans to merge and release?

argiepiano commented 1 year ago

Hi @herbdool and @yorkshire-pudding. Since this is RTBC, the PR was provided back in Dec, and @earlyburg has not responded to the ping in a few weeks, I can merge as a bug squad task. LMK