Make EntityMapper fallible

[cBournhonesque]

Objective

• Fixes Provide a fallible map_entity api #17193

Solution

Update the EntityMapper trait so that
map_entity(&mut self, entity: Entity) returns an Option<Entity> instead of Entity to account for cases where the mapping fails or cannot be done.

Testing

Updated the unit tests.
Maybe we should be using a Result, but I didn't know what to put in the error type. Maybe Box<dyn Error>?

Migration Guide

• The map_entity method of the EntityMapper trait now returns an Option<Entity> instead of an Entity to support cases where the mapping fails. You will probably need to update any custom implementations of the MapEntities trait.

[Shatur]

After thinking about it a bit more, I think we always should replace the entity with Entity::PLACEHOLDER instead of keeping it untouched.
If yes, do we need to return Option?

[cBournhonesque]

After thinking about it a bit more, I think we always should replace the entity with Entity::PLACEHOLDER instead of keeping it untouched. If yes, do we need to return Option?

So you're saying that you update MapEntities to return Entity::PLACEHOLDER if it fails,
and then you update EntityMapper to do something like:

    fn map_entities<M: EntityMapper>(&mut self, entity_mapper: &mut M) {
        let mapped = entity_mapper.map_entity(self.entity);
        if mapped != Entity::PLACEHOLDER {
            self.entity = mapped;
        }
    }

I think that works but it requires a bit of user-knowledge. It's kind of 'hidden' pattern that the user can remember to use Entity::PLACEHOLDER as an error value in both MapEntities and EntityMapper. I would probably prefer things to be more explicit.

[Shatur]

So you're saying that you update MapEntities to return Entity::PLACEHOLDER if it fails, and then you update EntityMapper to do something like:

No, I suggesting to always assign the entity even if it's Entity::PLACEHOLDER. Like it was before:

    fn map_entities<M: EntityMapper>(&mut self, entity_mapper: &mut M) {
        self.entity = entity_mapper.map_entity(self.entity);
    }

It's just better then pointing to some random entity. So I suggesting to edit the map_entity implementation instead.

[cBournhonesque]

So you're saying that you update MapEntities to return Entity::PLACEHOLDER if it fails, and then you update EntityMapper to do something like:

No, I suggesting to always assign the entity even if it's Entity::PLACEHOLDER. Like it was before:

    fn map_entities<M: EntityMapper>(&mut self, entity_mapper: &mut M) {
        self.entity = entity_mapper.map_entity(self.entity);
    }

It's just better then pointing to some random entity. So I suggesting to edit the map_entity implementation instead.

Ah I see; yes this works for my usecase!