Skip to content
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

[NFR] \Phalcon\Mvc\Model::columnMap() - Auto-map database columns that are not defined (like aliases) #1780

Closed
tilleps opened this issue Jan 10, 2014 · 12 comments

Comments

@tilleps
Copy link

tilleps commented Jan 10, 2014

Summary

Adding columns to a database table causes exceptions if those added columns were not defined in \Phalcon\Mvc\Model::columnMap() prior. This makes working with a live/production database difficult. I feel that columnMap() should feel and act like column aliases.

Database table schema:

Product
    product_id
    product_key
    product_label

Model

class Product extends \Phalcon\Mvc\Model
{
    public function columnMap()
    {
        return array(
            'product_id' => 'id',
            'product_key' => 'key',
            'product_label' => 'label',
        );
    }
}

Issue: Adding a column to database table

Product
    product_id
    product_key
    product_label
    product_description

Results in:
Phalcon\Mvc\Model\Exception: Column "product_description" doesn't make part of the column map

New Feature Request:
For database table columns that are not defined in columnMap(), could those columns automatically be added to the columnMap() (as if there was no column alias)?

class Product extends \Phalcon\Mvc\Model
{
    public function columnMap()
    {
        return array(
            'product_id' => 'id',
            'product_key' => 'key',
            'product_label' => 'label',

            // automatically add to columnMap()
            'product_description' => 'product_description',
        );
    }
}

_Suggested Solution_
In the following \Phalcon\Mvc\Model methods: assign(), cloneResultMap(), cloneResultMapHyrdate(), setSnapshotData()

$key = array_key_exists($key, $columnMap) ? $columnMap : $key;

Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.

@ghost
Copy link

ghost commented Jan 13, 2014

@phalcon what do you think if instead of

if (Z_TYPE_P(column_map) == IS_ARRAY) { 
    if (phalcon_array_isset(column_map, field)) {
        PHALCON_OBS_NVAR(attribute_field);
        phalcon_array_fetch(&attribute_field, column_map, field, PH_NOISY);
    } else {
        PHALCON_INIT_NVAR(exception_message);
        PHALCON_CONCAT_SVS(exception_message, "Column '", field, "' isn't part of the column map");
        PHALCON_THROW_EXCEPTION_ZVAL(phalcon_mvc_model_exception_ce, exception_message);
        return;
    }
} else {
    PHALCON_CPY_WRT(attribute_field, field);
}

we do something like

if (Z_TYPE_P(column_map) == IS_ARRAY && phalcon_array_isset(column_map, field)) { 
    PHALCON_OBS_NVAR(attribute_field);
    phalcon_array_fetch(&attribute_field, column_map, field, PH_NOISY);
} else {
    PHALCON_CPY_WRT(attribute_field, field);
}

?

@tilleps
Copy link
Author

tilleps commented Jan 13, 2014

I could see how some people may prefer the current behavior (or expect it to throw an exception), so perhaps checking/setting a default option (like columnMapOptional = false) when implementing this feature would be better going forward.

@phalcon
Copy link
Collaborator

phalcon commented Jan 13, 2014

I agree, we need some option to keep the original behavior as this is more strict and help the developer to find possible bugs.

@MatejBalantic
Copy link

I agree that the behavior is nice for strict development, however it is a great risk for production environments. For example, updating a database schema by adding a new column should never break the production code (not even for the split second). The migrations or annotated approach for schema are partly a solution, but it would also be nice to turn this off.

Or, at least consider throwing a specialized exception which can be cought and silently ignored. Right now model would throw Phalcon\Mvc\Model\Exception without any error-specific code, and even the message itself contains column name.

So, the only solution for me right now is catching Phalcon\Mvc\Model\Exception and searching for "isn't part of the column map" - which seems hacky ...

@oh-ren
Copy link

oh-ren commented May 24, 2014

Any news on this? Possible implementation?

I also would like to see this implemented. At times I only need to (re)map 1 or 2 columns (mostly having to do with adding '_id' to some column) (mainly to prevent clashing with some relationship alias).

(Some elaboration on my methods: I prefer to name my (foreign key) columns (in the db) to what feels natural to me, and at times that is without e.g. '_id' added to them (which then in most cases would effectively lead to them having the same name as the table they're referring to).)

It's tedious to have to add all columns in the columnMap array even if I don't want to remap them.

@cxfksword
Copy link

Any plan for this?

@blocksninja
Copy link

Any progress on this?
Would also like to seen an option to prevent an exception on new columns. Would be nice if we have the chance to ignore or auto-map them.

@samijnih
Copy link

👍 for all theses recommendations, I have the same problem and it would be nice if it could escape unimplemented columns in the columnMap method

@andresgutierrez
Copy link
Contributor

This is fixed in the 2.0.x branch (Phalcon 2.0.4), you have to change a global ORM setting to allow unexpected columns:

\Phalcon\Mvc\Model::setup(['ignoreUnknownColumns' => true]);

@blocksninja
Copy link

It seems that the "ignoreUnkownColumns" isn't working (anymore).
Or maybe it doesn't work when creating/saving an item. I've enabled the ignoreUnknownColumns, by code and by setting the php.ini setting. They are recognised, but seem to be ignored for saving the model.

@sergeyklay
Copy link
Contributor

@timbrand Create please separated issue with small script to reproduce

@wajdijurry
Copy link

+1

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

10 participants