SQL Mapper won't load next() record after erase()

[dexwerx]

Simple code snippet:

$mapper = new \DB\SQL\Mapper($db,$table); 
$auditer = new \Observor($mapper);
$mapper->load(['fk=?',1]);
while ($mapper->valid()) { 
    $mapper->erase(); 
    $mapper->next(); 
} 

it works as expected if you comment out the erase() Doing it this way, so next() calls the onload trigger for every record sequentially. using find or select - calls the onload trigger for all records at once, wrather than sequentially in the loop.

[bcosca]

I made some changes to the Cursor class.

What is expected behavior? If you had 3 rows and the current position is 1 (zero-based), then when you erase that row, that position shouldn't move because your cursor pointer is still at 1 but it points at what used to be row 3 which is now row 2. Therefore, next() should be unnecessary.

[dexwerx]

intuitively the cursor position shouldn't change automatically once a row is deleted. The cursor position (despite being invalid) is still pointing to the erased, blank record. It might be easiest to just modifiy next() to get the desired behavior, without having to consider stale cursor positions. I'll have to refer to other APIs to see what they do. (this is how microsoft active record style apis work)

<?php
while ($mapper->valid()) { 
    $mapper->erase(); 
    echo $mapper->dry(); // TRUE (1)
    $mapper->name = 'New Record';
    $mapper->next(); 
    echo $mapper->dry(); // FALSE (0)
} 

Thanks for taking the time to look at it!

I can see the row is automatically removed from the recordset upon \DB\Cursor::erase(). This is great for simplicity, but at the cost of intuitive behavior.

Laravel Eloquent seems to get around this by configuring the model triggers globally, and then loads another duplicate model instance of the current row (one at a time) to properly call triggers, before deletion. I don't think it manages or even cares what happens to the now invalid row.

I'm just going to comment out db/cursor.php in my project.

245 //      $this->query=array_slice($this->query,0,$this->ptr,TRUE)+
246 //          array_slice($this->query,$this->ptr,NULL,TRUE);

or better yet just override erase or use a destroy method as part of my base model.

Please note that the 'beforeerase' trigger is called after unsetting all the fields. It should be called before unsetting the fields.

Regards!

[dexwerx]

Here's what I did to erase() in my base Model.

    function erase($filter=NULL) {
        if ($filter)
            return parent::erase($filter);
        $args=array();
        $ctr=0;
        $filter='';
        $pkeys=array();
        foreach ($this->fields as $key=>&$field) {
            if ($field['pkey']) {
                $filter.=($filter?' AND ':'').$this->db->quotekey($key).'=?';
                $args[$ctr+1]=array($field['previous'],$field['pdo_type']);
                $pkeys[$key]=$field['previous'];
                $ctr++;
            }
            unset($field);
        }
        if (isset($this->trigger['beforeerase']))
            \Base::instance()->call($this->trigger['beforeerase'],
                array($this,$pkeys));
        $out=$this->db->
            exec('DELETE FROM '.$this->table.' WHERE '.$filter.';',$args);
        if (isset($this->trigger['aftererase']))
            \Base::instance()->call($this->trigger['aftererase'],
                array($this,$pkeys));
        return $out;
    }

[dexwerx]

Just wanted to add that Yii Active Record behaves similar to Laravel, and completely leaves the current record instance alone, even after its deleted from the database. They don't even blank all the fields. essentially there is no action required other than actually deleting the record from the DB.

[bcosca]

When you add a record with an autoincrement ID, you expect things to be done automatically for you. With F3, after a $mapper->save() you can immediately use $mapper->_id (autoincrement ID) right after the command. It didn't tell you to do the mechanical process of using load() again just so you can get the ID.

If that is done automatically for you, why should the framework behave differently for the deletion process. If one were to delete a record, one should also expect the same dynamic behavior as the append scenario from the F3 mapper. In other words, a mapper should always point to a record. What you propose is that the pointer be left in an UNKNOWN state, pointing to nothing. Getting values from a deleted record doesn't make sense either.

IMO, the ORM representations of the frameworks that you mentioned are too mechanical. They have a narrow view of the ActiveRecord pattern. They forgot the meaning of "Active". Theirs is just a static "Record" representation of data objects.

F3 gives you the convenience of retrieving the schema for you. That's one of the things "Active" should mean. You didn't have to write the structure of your table in PHP, which means duplicating the SQL table definition.

[KOTRET]

i agree that this would be a bad behavior. This would allow to call other mapper functions that wont work like another erase or insert / update. @bcosca: a little off topic: the mapper will assign the autoincrement to _id, but the record inserted is still not usable for update - you need to load it again. Imho this should get fixed for single PK-Entities (actually i don't know the return value of auto key for multicolumn).

[xfra35]

@KOTRET are you sure about it? There's a reload in insert(). See https://github.com/bcosca/fatfree-core/blob/master/db/sql/mapper.php#L431.

[KOTRET]

@xfra35: i was not :) mhm, reasonable because of maybe existing database triggers. controversal

[dexwerx]

If one were to delete a record, one should also expect the same dynamic behavior as the append scenario from the F3 mapper. In other words, a mapper should always point to a record.

except when you create a new mapper with a base table? The mapper isn't conceptually pointing to any record until you save()

What you propose is that the pointer be left in an UNKNOWN state, pointing to nothing.

I see this is the most practical solution when dealing with any shared dynamic database.

Getting values from a deleted record doesn't make sense either.

I aggree. As a matter of practicality other APIs define anything you do with a 'deleted' record as undefined, and don't bother doing anything beyond that. Some API's will go the extra step and blank the record and even throw an exception if you try and do anything with the values. I would say just blanking the record would be a good enough. It doesn't make sense to try and define the behavior of using a deleted record. To me a save() on a deleted would recreate the record as new, seems consistent with f3, but I would be stretching for that. It's also not practical to try and safeguard actions you might perform on a deleted record.

IMO, the ORM representations of the frameworks that you mentioned are too mechanical. They have a narrow view of the ActiveRecord pattern. They forgot the meaning of "Active". Theirs is just a static "Record" representation of data objects.

This is where I would dissagree with you and go with the consensus. Practically speaking, it's impossible to have Active Records as you define Active. This is not what is meant by Active Record. When dealing with dynamic shared databases, the moment you open a record set / call load() the data you are dealing with is by definition static. This is exactly why f3 has a loaded() and a count(). I don't think this understanding is mechanical - I think its the only practical way to deal with dynamic databases. But don't take my word for it, or even the 2 most popular ORMs I sited.

while ($mapper->valid()) { 
    $mapper->erase(); 
    $mapper->next(); 
} 

This creates an infinite loop in f3. It's not intuitive. It's bad behavior. I wouldn't think this would be up for debate.

Kudos to the design though - the f3 ORM is so simple, that overriding erase() in a base model to make it more inline with decades of industry standard is easy, without modifying the framework. That alone makes f3 well worth the quirks and bad behavior to me.

[bcosca]

I tagged this as a question so the community can provide feedback/opinion.

[xfra35]

@dexwerx, maybe I've missed something but didn't @bcosca implemented what you've been asking for?

1. $mapper->erase() doesn't change the cursor position anymore
2. the following code doesn't create an infinite loop:

while ($mapper->valid()) { 
    $mapper->erase(); 
    $mapper->next(); 
}

[dexwerx]

Great, I just downloaded the latest snapshot, and I can confirm that the loop is well behaved. This issue can be closed/resolved.

Thanks!