Skip to content

Replace some references to "entity" term with "model"#6108

Merged
VincentLanglet merged 1 commit intosonata-project:3.xfrom
phansys:deprecations_4
Jun 10, 2020
Merged

Replace some references to "entity" term with "model"#6108
VincentLanglet merged 1 commit intosonata-project:3.xfrom
phansys:deprecations_4

Conversation

@phansys
Copy link
Member

@phansys phansys commented May 24, 2020

Subject

Replace some references to "entity" term with "model" in order to enforce the agnosticism around the model manager.

I am targeting this branch, because these changes respect BC.

Changelog

### Deprecated
- Deprecated `FieldDescriptionInterface::getTargetEntity()` in favor of `FieldDescriptionInterface::getTargetModel()`;
- Deprecated `AdminHelper::getEntityClassName()` in favor of `AdminHelper::getModelClassName()`;
- Deprecated `GenerateObjectAclCommand::getUserEntityClass()` in favor of `GenerateObjectAclCommand::getUserModelClass()`.
- Deprecated `--user_entity` option in favor of `--user_model` at `sonata:admin:generate-object-acl` command.

@phansys phansys added the minor label May 24, 2020
@phansys phansys force-pushed the deprecations_4 branch 4 times, most recently from 168ae18 to 4746d36 Compare May 25, 2020 00:41
@phansys phansys requested a review from a team May 25, 2020 00:50
@phansys phansys marked this pull request as ready for review May 25, 2020 00:50
@phansys phansys requested review from a team, VincentLanglet and core23 May 25, 2020 14:48
VincentLanglet
VincentLanglet previously approved these changes May 30, 2020
@VincentLanglet
Copy link
Member

@phansys If you want to fully enforce the agnosticism around the model manager, we have to do something about the following lines

return $associationMapping['fieldName'].'.';

$method = sprintf('get%s', $inflector->classify($parentMapping['fieldName']));

$method = sprintf('add%s', $inflector->classify($inflector->singularize($mapping['fieldName'])));

We're trying to access to the 'fieldName' key.

If we have a getter to avoid using the key targetEntity, do we need to do similar for fieldName ? The difficulty here is that is an array of array parentAssociationMappings. Creating a getParentAssociationMappingsFieldNames seems weird to me.

Should we create something like an FieldMappingAdapter and a AssociationMappingAdapter with methods

  • getFieldName(array $mapping)
  • getIdentifier(array $mapping)
  • getFieldType(array $mapping)
  • getModelClassName(array $mapping)
    Then
    A FieldDescription will expose a getAssociationMappingAdapter and won't expose a getModelClassName because you'll call $fieldDescription->getAssociationMappingAdapter->getModelClassName($fieldDescription->getAssociationMapping()).

I maybe can implements this for the next major, since I started something here sonata-project/SonataDatagridBundle#234

WDYT ? Do you have any idea to improve the situation ?

@phansys
Copy link
Member Author

phansys commented May 31, 2020

@phansys If you want to fully enforce the agnosticism around the model manager, we have to do something about the following lines

I agree, but the intention of my PR is not so ambitious. I just wanted to be Doctrine model agnostic, replacing any reference to entity (ORM), document (ODM), etc.

I think our next goal should be exactly what you're proposing. In some private projects I'm using this package to handle models from ReST APIs or things like that, but given these limitations I found myself making some nasty tricks in order to make it work.

WDYT ? Do you have any idea to improve the situation ?

I've to analyze our API again in order to determine which paths I can identify currently, but IMO the approach you've delineated is in the right way.

…orce the agnosticism around the model manager
@VincentLanglet VincentLanglet requested a review from a team June 9, 2020 21:29
@VincentLanglet VincentLanglet merged commit e0aa21d into sonata-project:3.x Jun 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants