Entry references mappers are hiding bugs

[e1himself]

This catch-all exception inside EntryHyperlink and other mappers implementation is hiding any error occurring during the link resolution.

Which may hide dangerous bugs and makes it nearly impossible to debug problems (see https://github.com/contentful/rich-text.php/issues/43).

https://github.com/prezly-forks/contentful-rich-text.php/blob/6ac52e25334ab9a43e08c849149c2f91ead66616/src/NodeMapper/EntryHyperlink.php#L31-L38

try {
    /** @var EntryInterface $entry */
    $entry = $linkResolver->resolveLink(
        new Link($linkData['id'], $linkData['linkType'])
    );
} catch (\Throwable $exception) {
    throw new MapperException($data);
}

[e1himself]

I've prepared a PR to fix it. Here: #45

[pgrigoruta]

Hi @e1himself , thanks for noticing this!

The MapperException repackaging can indeed catch unintended behaviour. In PR https://github.com/contentful/rich-text.php/pull/46 (which should be merged soon), I am adding the original exception to the Mapper Exception. This should be semantically correct, i.e. everything generated by that part of code is a Mapper Exception, but now you have more information as into what it was, so you can write conditional code on your client.

My main motivation of not leaving the exception unpacked (i.e. keeping the original class), is that other library users might rely on the MapperException type, so hopefully this implementation is the best of both worlds.

[e1himself]

For future reference: this is fixed with #47

[e1himself]

For future reference.

The MapperException repackaging can indeed catch unintended behaviour. In PR #46 (which should be merged soon), I am adding the original exception to the Mapper Exception. This should be semantically correct, i.e. everything generated by that part of code is a Mapper Exception, but now you have more information as into what it was, so you can write conditional code on your client.

My main motivation of not leaving the exception unpacked (i.e. keeping the original class), is that other library users might rely on the MapperException type, so hopefully this implementation is the best of both worlds.

I've replied to the above comment in the PR:

https://github.com/contentful/rich-text.php/pull/46#issuecomment-583379559