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

Cannot override a field definition #127

Closed
chanmix51 opened this issue Feb 13, 2014 · 2 comments
Closed

Cannot override a field definition #127

chanmix51 opened this issue Feb 13, 2014 · 2 comments
Labels

Comments

@chanmix51
Copy link
Owner

In 1.1, it was possible to override a field definition. The RowStructure class prevents that:

// MyEntityMap
  public function initialize()
  {
    parent::initialize();
    $this->addField('my_field', 'new_type'); 
  }

This throws an exception if the "my_field" field is already assigned with a type in the base class. This is a bug because it was not the case in 1.1 and I think it should be possible.

There are several ways of solving that problem:
1° Create an "overrideField" method
2° Add a $force flag to addField (default to false)
3° Remove the exception from addField (like it was in 1.1)

Any thoughts ?

@chanmix51 chanmix51 added the bug label Feb 13, 2014
@ericrisler
Copy link

I think #3 - Why was the exception added there in the first place?

Is it dangerous to override the field definition? Would it lead to instability or undefined behaviour?

If the exception is there to notify the programmer that they may be doing something wrong I vote remove it altogether and document the behaviour in your examples. Removing it would allow for the programmer to redefine the field type to something custom and setup a Converter to handle the data. (see https://groups.google.com/forum/?fromgroups=#!topic/pommproject/FaCGbC2p1Bg).

@chanmix51
Copy link
Owner Author

I do agree on option 3. We can trust the the generated classes since the generator is tested. If the programer wish was to add a non db existent field, he would use addVirtualField so using addField means "add or replace definition".
Last, this change does not create a compatibility problem nor a regression but is is not a new feature either (stable branch policy).

chanmix51 added a commit that referenced this issue Feb 13, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants