laravel / framework

The Laravel Framework.
https://laravel.com
MIT License
32.39k stars 10.98k forks source link

[BUG] [5.5] artisan queue:retry failing when FETCH MODE is ASSOC. #23040

Closed rk closed 6 years ago

rk commented 6 years ago

Description:

Because the application was recently upgraded to Laravel 5.5, I haven't had time to switch to FETCH_OBJ. I had to use the FETCH_ASSOC for compatibility until I get the time to rewrite a few hundred lines of code.

The job queue cannot be retried because:

In RetryCommand.php line 70:

  [ErrorException]
  Trying to get property of non-object

Steps To Reproduce:

You must switch the PDOStatement fetch mode to FETCH_ASSOC.

// From App\EventServiceProvider::boot
Event::listen(\Illuminate\Database\Events\StatementPrepared::class, function ($event) {
  $event->statement->setFetchMode(\PDO::FETCH_ASSOC);
});

Architect a job that purposefully throws an exception, and queue it.

<?php

namespace App\Jobs;

use Illuminate\Bus\Queueable;
use Illuminate\Queue\SerializesModels;
use Illuminate\Queue\InteractsWithQueue;
use Illuminate\Contracts\Queue\ShouldQueue;
use Illuminate\Foundation\Bus\Dispatchable;

class TestFailureJob implements ShouldQueue
{
    use Dispatchable, InteractsWithQueue, Queueable, SerializesModels;

    /**
     * Execute the job.
     *
     * @return void
     */
    public function handle()
    {
        throw new \RuntimeException("This job is meant to fail.");
    }
}

I queue it via php artisan tinker, myself:

~/path/to/project $ artisan tinker
Psy Shell v0.8.17 (PHP 7.0.27 β€” cli) by Justin Hileman
>>> \App\Jobs\TestFailureJob::dispatch()->delay(now()->addSeconds(10))
=> Illuminate\Foundation\Bus\PendingDispatch {#1055}

Run the job so it lands in the failed_jobs table, and then retry it...

php artisan queue:work --tries=1
php artisan queue:retry all

πŸ’₯ πŸ’₯ πŸ’₯

In RetryCommand.php line 70:

  [ErrorException]
  Trying to get property of non-object

Mitigation

Here's how I'm mitigating the issue: by checking if the failed_jobs or jobs tables are being selected from.

Event::listen(\Illuminate\Database\Events\StatementPrepared::class, function ($event) {
  /** @var \PDOStatement $stmt */
  $stmt = $event->statement;

  if (!preg_match('/ FROM `?(failed_jobs|jobs)`?/i', $stmt->queryString)) {
    $stmt->setFetchMode(\PDO::FETCH_ASSOC);
  }
});

Long-term, a patch to fix the RetryCommand would be better than this regex test. I thought I saw something about this on laravel/internals a little while ago, but I can't find it now.

mfn commented 6 years ago

I don't want to sound too negative, but I don't see how this is a bug in Laravel.

There's a reason changing the "fetch mode" was made harder in 5.4 due to all the problems it entails, like the one you discovered.

I can't see how any Laravel framework part can be sanctioned compatible with nonFETCH_OBJ mode πŸ€·β€β™€οΈ

rk commented 6 years ago

Considering that the fetch mode was FETCH_ASSOC as recently as 5.3, the change to FETCH_OBJ should have entailed a 6.0 version number--as backward compatibility is affected. Because there's no way to execute a query with FETCH_ASSOC it breaks compatibility with user-land code. There are efficiency reasons why fetching an array is beneficial, so the reason to remove support for it completely is puzzling.

If the framework parts can't be considered compatible in non FETCH_OBJ mode, then those of us who need access to an alternative fetch mode need a way to set it on queries from the DB facade. Otherwise, we're reduced to directly accessing PDO.

mfn commented 6 years ago

Considering that the fetch mode was FETCH_ASSOC as recently as 5.3, the change to FETCH_OBJ should have entailed a 6.0 version number--as backward compatibility is affected

That would be true if Laravel would follow semantic versioning. Which, as the owners/members have stated multiple times, it dosn't … :|

To make it short: any bump in minor version number is entailed for breaking changes (and in fact does so, as I've experienced myself multiple times).

See also https://github.com/laravel/framework/issues/22833 if you're interested in more recent discussion about a similar topic.

Sorry I don't have more positive stuff to contribute here.

laurencei commented 6 years ago

I'm going to close this - as it's not a bug with the Framework - just a byproduct of upgrading to a newer version of Laravel.

konohanaruto commented 5 years ago

Check out upgrade guide: (laravel 5.4 upgrade)

Fetch Mode Laravel no longer includes the ability to customize the PDO "fetch mode" from your configuration files. Instead, PDO::FETCH_OBJ is always used. If you would still like to customize the fetch mode for your application you may listen for the new Illuminate\Database\Events\StatementPrepared event:

Event::listen(StatementPrepared::class, function ($event) {
    $event->statement->setFetchMode(...);
});

The solution is as follows:

from  -->  app/Providers/EventServiceProvider.php

use Illuminate\Database\Events\StatementPrepared;
use Illuminate\Support\Facades\Event;

// ...

public function boot()
{
    parent::boot();
    Event::listen(StatementPrepared::class, function ($event) {
        $event->statement->setFetchMode(\PDO::FETCH_ASSOC);
    });
}

If it is lumen, make sure that the Provider class is loaded in your bootstrap\app.php file.

// ...
$app->register(App\Providers\EventServiceProvider::class);
// ...
rk commented 5 years ago

@konohanaruto That's exactly what I did, and it caused the problem I reported in the original ticket entry._The problem was that FETCH_ASSOC and the job queue aren't compatible; just create a test project from Laravel 5.4 and follow my steps to reproduce. The upgrade guide never mentioned that the "fix" could break the core functionality, and especially not the queue. I caught this issue in production (our jobs don't often fail), and documented it for anyone else maintaining a 5.4 app.

I see no reason for this discussion to continue. Maybe @laurencei can lock this?

konohanaruto commented 5 years ago

@rk Can you paste your core code, maybe it's easier to help locate the problem?