-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Description
There are packages that use ClassMetadata to programmatically generate entity forms or admin UIs. Examples are Symfony Doctrine bridge and EasyAdminBundle.
The API for checking if an entity property is representing a) a field or b) an embedded class or c) an association is:
class ClassMetadata
{
public $fieldMappings;
public $embeddedClasses;
public $associationMappings;
public function hasField($fieldName)
{
return isset($this->fieldMappings[$fieldName]) || isset($this->embeddedClasses[$fieldName]);
}
public function hasAssociation($fieldName)
{
return isset($this->associationMappings[$fieldName]);
}
}a) The API is not uniform and packages use:
isset($metadata->fieldMappings[$propertyName])-> to know if a property represents a field (and not an embeddable class)isset($metadata->embeddedClasses[$propertyName])-> to know if a property represents an embedded class (and not a field)$metadata->hasAssociation($propertyName)-> to know if a property represents an association
b) The API is confusing IMO:
$metadata->hasField($propertyName)returns true if there is no field mapping but an embedded class mapping (which I guess makes sense in some contexts but I didn't find such a context even though I searched for one and even if it exists two seperate checks can still be done there)
I searched Doctrine's code and both methods hasAssociation and hasField are not used often, most time the issetcheck is used.
Would you accept a PR for ORM 2 that deprecates both methods and suggest to uniformly use the isset check in all client code? (Also affects the interface Doctrine\Persistence\Mapping\ClassMetadata). So client code is never tempted to sometimes use the methods and sometimes use the isset way just because developers don't know what is the better way and if there is a better way at all.
class ClassMetadata
{
/** @deprecated */
public function hasField($fieldName) {...}
/** @deprecated */
public function hasAssociation($fieldName) {...}
}Or would you accept a refactoring PR for ORM 2 that deprecates 2 methods and adds 3 other methods? (Also in the interface Doctrine\Persistence\Mapping\ClassMetadata).
class ClassMetadata
{
/** @deprecated */
public function hasField($fieldName) {}
/** @deprecated */
public function hasAssociation($fieldName) {}
public function hasFieldMapping($fieldName)
{
return isset($this->fieldMappings[$fieldName]);
}
public function hasEmbeddedClassMapping($fieldName)
{
return isset($this->embeddedClasses[$fieldName]);
}
public function hasAssociationMapping($fieldName)
{
return isset($this->associationMappings[$fieldName]);
}
}I would look forward to implement it and refactor existing usages but I guess changing an interface is not very welcome. Maybe it's not worth the effort to do a PR? Thanks a lot for any answer.