laravel / ideas

Issues board used for Laravel internals discussions.
938 stars 28 forks source link

Better exception management in Cache/DatabaseStore::put #1468

Open alessiodebernardi opened 5 years ago

alessiodebernardi commented 5 years ago

Why the method putdeclared inside the class Illuminate/Cache/DatabaseStore catches a general Exception and immediately assumes that is a "duplicate key" case and do an update query? What happen if the exception refers to other? In my case, for example, using a MySQL database, i found out a "String data, right truncated: 1406 Data too long for column..." exception but it was very difficult to debug this, because the method don't report/log/throw anything.

  /**
     * Store an item in the cache for a given number of minutes.
     *
     * @param  string  $key
     * @param  mixed   $value
     * @param  float|int  $minutes
     * @return void
     */
    public function put($key, $value, $minutes)
    {
        $key = $this->prefix.$key;
        $value = $this->serialize($value);
        $expiration = $this->getTime() + (int) ($minutes * 60);
        try {
            $this->table()->insert(compact('key', 'value', 'expiration'));
        } catch (Exception $e) {
            $this->table()->where('key', $key)->update(compact('value', 'expiration'));
        }
    }

(Sorry for my bad english)

mfn commented 5 years ago

Hmm, indeed doesn't seem "nice" for developers. I see two immediate options but also challenges:

  1. catch PDOException and check for error code specifically, re-throw if it's not a unique violation
  2. use upsert logic

Both AFAICS have the same challenge: not cross-database compatible (error codes vary between database as does the upsert syntax; the latter may not even be supported by all databases).

Given this, I think I can understand why the code is written that way it is: not ideal, but works for 99.9% ; you happen to be the unlucky 0.1% :-/