Added a check for configured field name conflict with inherited methods #9549
Conversation
jakxnz
force-pushed
the
pulls/4/guard-against-field-vs-parent-method-conflict
branch
from
e8bff5d
to
2a59b1b
Compare
| @@ -3451,6 +3452,17 @@ public function requireTable() | |||
| ); | |||
| } | |||
|
|
|||
| // Sanity check for fields that conflict with parent | |||
| foreach (array_keys($fields) as $fieldName) { | |||
| if (method_exists(get_parent_class($this), $fieldName) && !($this instanceof TestOnly)) { | |||
There was a problem hiding this comment.
Shouldn't we use singleton(get_parent_class($this))->hasMethod($fieldName)) so that we can check for methods added by extensions?
this does also pose the question of how we handle extensions that are added after the model has been defined. eg: adding an extension as a feature request on top of a pre-existing site
There was a problem hiding this comment.
🤔 True that. And maybe reduce the interrupt to a warning (similar to DBSchemaManager::requireTable()?) and add something like a $ignore_conflicting_fields = [] config to give users the ability to suppress the warning?
There was a problem hiding this comment.
@dhensby any thoughts?
There was a problem hiding this comment.
The fact that there's a need to block this on test classes is a bit of a code smell: why is that needed and why would it never be needed in a non-test case?
There was a problem hiding this comment.
Also what about the "get$fieldName" method?
There was a problem hiding this comment.
Should there be similar checks for relations?
There was a problem hiding this comment.
+1 for reducing the error to a warning. With that in place I think we can avoid the need for a suppression filter.
This does also pose the question of how we handle extensions that are added after the model has been defined.
You're going to have a conflicting situation. Right now the system blindly lets you build a corrupt, inconsistent app. Warning that you're doing this but leaving behaviour otherwise unchanged seems fine. In such a case a developer would be advised to refactor their site to avoid conflicts, which would be the best way to get rid of the warning message. Otherwise they can suppress warnings application-wide if they're that much of a cowboy ;)
| @@ -3451,6 +3452,17 @@ public function requireTable() | |||
| ); | |||
| } | |||
|
|
|||
| // Sanity check for fields that conflict with parent | |||
| foreach (array_keys($fields) as $fieldName) { | |||
| if (method_exists(get_parent_class($this), $fieldName) && !($this instanceof TestOnly)) { | |||
There was a problem hiding this comment.
Also what about the "get$fieldName" method?
| @@ -3451,6 +3452,17 @@ public function requireTable() | |||
| ); | |||
| } | |||
|
|
|||
| // Sanity check for fields that conflict with parent | |||
| foreach (array_keys($fields) as $fieldName) { | |||
| if (method_exists(get_parent_class($this), $fieldName) && !($this instanceof TestOnly)) { | |||
There was a problem hiding this comment.
Should there be similar checks for relations?
#6329
Added a sanity check which throws an error if a configured
$dbfield has the same name as a method in the parent-level scope. IgnoresTestOnlyclasses