Issues

ZF-2658: More Flexible Mapping For Zend_Log_Writer_Db

Description

In my opinion, the mapping logic is too rigid. If there is a mapping present, the insertion of log messages includes those fields and only those fields each and every time. This produces undesired results when:

New fields are added using the setEventItem method and there is no mapping for them. These fields will never be inserted.

When fields are empty and must be inserted as NULL, insertion failure. Since the fields are all named, these should just be skipped.

Fields that don't need to be mapped still must have a mapping entry, just so it is included ('message' => 'message').

I think most people would use mapping as an overload of field names. That would address all the issues above. Fields that are not overloaded (name wise) would be inserted as is. Something like the following:

Database ( creation, priority, message );

Mapping = array('creation' => 'timestamp');


if ($this->_columnMap === null) {
    $dataToInsert = $event;
} else {
    $dataToInsert = array();
    foreach ($this->_columnMap as $columnName => $fieldKey) {
        if (isset($event[$fieldKey])) { $dataToInsert[$columnName] = $event[$fieldKey]; }
        unset($event[$fieldKey]);
    }
    if (count($event)) {
        foreach ($event as $eventKey => $eventVal) {
            $dataToInsert[$eventKey] = $eventVal;
        }
    }
}

One final thought. In my opinion, the mapping logic should be reversed. So, instead of mapping database field names to log event items, it should be log event items mapped to database fields. Its not really a big deal, because you can just flip the mapping array, but I think it is more intuitive.

Comments

This seems like a worthy change. Arthur, have you signed a CLA so that we can use the code above? Also, can you write the necessary unit tests and add the necessary tables to the test schema?

I have not signed a CLA. I will get that in.

I have no experience with unit tests.

Reassigned to component maintainer

@Arthur, You have not yet signed the CLA apparently.