Skip to content

Eloquent ORM goes OOM on delete() for a model without primary key #3761

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
DSpeichert opened this issue Mar 3, 2014 · 10 comments
Closed

Eloquent ORM goes OOM on delete() for a model without primary key #3761

DSpeichert opened this issue Mar 3, 2014 · 10 comments

Comments

@DSpeichert
Copy link
Contributor

For a model with:

protected $primaryKey = null;

when $instance->delete() is called, it fails with out of memory fatal error.

@anlutro
Copy link
Contributor

anlutro commented Mar 3, 2014

That's odd, but it's worth noting that delete() is not designed to work at all without a primary key.

@Anahkiasen
Copy link
Contributor

What column is it supposed to delete with ?

@DSpeichert
Copy link
Contributor Author

If there is no primary key, it could delete specifying all columns. In any case, it should not fail with OOM but with a more meaningful error.
Still, using all columns to delete instance of a model without a primary key is the most anticipated action unless specifying an array for $primaryKey is possible.

@anlutro
Copy link
Contributor

anlutro commented Mar 3, 2014

If you were to implement that, (new MyModel())->delete() would delete the entire table, and you would often risk unintentional deletes. I think that would be a pretty bad idea.

@DSpeichert
Copy link
Contributor Author

That is true but it's not like you can accidentally put this into the code. Cases of models without primary key are rare anyway.

How about implementing $primaryKey being an array to support MySQL's combined primary keys?

@manan-jadhav
Copy link
Contributor

@DSpeichert , you can use an array as a PK.

@JosephSilber
Copy link
Contributor

@anlutro Checking $this->exists first would mitigate the (new MyModel())->delete() problem.

(not that I think this should be implemented. Just saying).

@taylorotwell
Copy link
Member

Fixed.

@YOzaz
Copy link

YOzaz commented Jun 16, 2014

Still doesn't work.

class Article extends \Eloquent
{
    use \Illuminate\Database\Eloquent\SoftDeletingTrait;
    public $incrementing = false;
    protected $primaryKey = [
        'article_id',
        'publisher_id'
    ];
$item = Article::where('article_id', 1)->where('publisher_id', 1)->first();
// works fine
$item->delete();
// throws Exception

Result:

ErrorException
array_key_exists(): The first argument should be either a string or an integer
\vendor\laravel\framework\src\Illuminate\Database\Eloquent\Model.php:2297
public function getAttribute($key)
{
    $inAttributes = array_key_exists($key, $this->attributes);

@lazlo-bonin
Copy link

@YOzaz is right, I get the same exception.

I think Eloquent expects a string primary key to delete there, and can't handle cases where the primary key is a composite defined through an array (where it should add multiple constraints in the where).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants