Contrib.social

Allow redirect_callback of batch_process to be a method and to return a value

Open on Drupal.org →
Created on 10 August 2016, over 8 years ago
Updated 15 September 2023, over 1 year ago

Problem/Motivation

Function batch_process takes $redirect_callback as optional third argument, which is pretty useless right now:

function batch_process($redirect = NULL, Url $url = NULL, $redirect_callback = NULL) {
  ...
      if (($function = $batch['redirect_callback']) && function_exists($function)) {
        $function($batch_url->toString(), ['query' => $query_options]);
      }
      else {
        return new RedirectResponse($batch_url->setAbsolute()->toString(TRUE)->getGeneratedUrl());
      }
...

If $redirect_callback is NULL or invalid we get a RedirectResponse in return, if it's set and valid we get ... nothing.

Given that in Drupal 7 redirect_callback used to default to drupal_goto(), it's perfectly clear why it was ported like this and why it's wrong: We need to return a response object or at least some data (i.e. the batch_id) here to do something useful.

As a bonus: Testing for callability with function_exists() was fine in the good old procedural days, but is really not sufficient nowadays.

Patch follows. The patch will fix this for the procedural code still in place, but I found the same problems in the code of #2320469: Move batch processing to a service → .

Steps to reproduce

Proposed resolution

Remaining tasks

Update issue summary, see #27
Clarify what is being fixed here.

User interface changes

API changes

Data model changes

Release notes snippet

🐛 Bug report
Status

Needs work

Version

9.5

Component
Batch  →

Last updated 4 days ago

Created by

🇩🇪Germany g.oechsler

Live updates comments and jobs are added and updated live.
  • Needs issue summary update

    Issue summaries save everyone time if they are kept up-to-date. See Update issue summary task instructions.

Sign in to follow issues

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • Comment almost 2 years ago →
    needs-review-queue-bot

    The Needs Review Queue Bot → tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

    Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

    Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

  • Comment almost 2 years ago →
    🇳🇿New Zealand quietone

    This was a bugsmash daily target a few days ago.

    larowlan, noted it need a reroll and that this may be a task not a bug. I can't comment on that but I will update the issue meta data. I am not tagging for a reroll because of comment #27 and #31. The patches have tests, so removing that tag.

  • Status changed to Needs review almost 2 years ago
  • Comment almost 2 years ago →
    🇮🇳India Abhisheksingh27

    Adding Reroll for 9.5.x.
    Was unable to add changes to " a/core/modules/system/src/Tests/Batch/ProcessingTest.php" as "ProcessingTest.php" was not present at the given path.

  • Comment almost 2 years ago →
    🇳🇿New Zealand quietone

    @Abhisheksingh27, Thank you for your assistance on this issue. However, this issue is not tagged for a reroll and it does not need one as explained in the Issue Summary and in #40. To receive credit for contributing to this issue, assist with other outstanding tasks which in this case is an Issue Summary update. See the issue credit guidelines → for more information.

    For others working on this issue the patch in #41 is a reroll of the test only patch in #27 and does not include the suggested fix.

  • Status changed to Needs work almost 2 years ago
  • Comment almost 2 years ago →
    🇮🇳India Abhisheksingh27

    @quietone Sorry for the inconvenience. Seems like i missed #40 comment.

  • Comment over 1 year ago →
    🇦🇹Austria hudri Austria

    I'm not totally sure if this is directly related, but batch_set() does not work if the submitting form has a ?destination GET parameter.

    I initially stumbled on this in the bookable_calendar module ( #3387507 🐛 Editing opening entites does not trigger batch process updating the instances Active ), and it looks suspiciously related to this issue.

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024