Problem/Motivation

  1. Whenever SparqlEntityStorageConverter triest to convert an value with an entity type with a non-existing entity type ID, it causes an exception instead of returning NULL (so, users see a 5xx error instead of a 404).
  2. Because the conversion is able to load entities with different storage than SparQL, it unnecessarily dispatches the SparqlEntityStorageEvents::GRAPH_ENTITY_CONVERT event.

Steps to reproduce

This was discovered with a path foo/{entity_type}/{entity} where the entity type is dynamic, and one tries to load the path /foo/missing_type/something. If this module is not installed, then visitors get a 404 response.

Expected: we get a 404 response (because of parameter is not converted)
Actual result: 500 response (PluginNotFoundException).

Proposed resolution

  1. When the entity type is known, so ::applies() can be fully evaluated: check whether there is a definition for the given entity type ID before trying to load the storage. It makes no sense in the current order.
  2. Do the same in ::convert when the entity type is dynamic, (so only ::applies is not invoked)

Remaining tasks

  1. Test
  2. Fix
  3. Review

User interface changes

Nothing.

API changes

SparqlEntityStorageEvents::GRAPH_ENTITY_CONVERT is only fired if a SparQL-stored entity is converted.

Data model changes

Nothing.

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

huzooka created an issue. See original summary.

huzooka’s picture

Assigned: huzooka » Unassigned
Status: Active » Needs review
huzooka’s picture

Issue summary: View changes
huzooka’s picture

Assigned: Unassigned » huzooka
Status: Needs review » Needs work
huzooka’s picture

Assigned: huzooka » Unassigned
Status: Needs work » Needs review

Should be perfect now 😬

dxvargas’s picture

Status: Needs review » Reviewed & tested by the community

The code seems good. I've tested in a vanilla install, ran the tests before and after... works as expected.

claudiu.cristea made their first commit to this issue’s fork.

claudiu.cristea’s picture

Thank you

joevagyok’s picture

Status: Reviewed & tested by the community » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.