rdf_entity_load() issues db queries whenever an entity is loaded.
In D8, that happens an *awful* lot of times on stuff rdf module shouldn't bother with...

Comments

yched’s picture

Status: Active » Needs review
StatusFileSize
new2.22 KB

Patch

Status: Needs review » Needs work

The last submitted patch, rdf-renderable_entities-1920498.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, rdf-renderable_entities-1920498.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
StatusFileSize
new4.62 KB

What about using a typed exception and catch that one?

yched’s picture

Yeah, getRenderController() and the other getController() methods are not totally ready for what we want to do here.
Raising and catching exceptions in regular request flow (like, here, on every ConfigEntity load...) is something we typically try to avoid.

- We should be able to call getController() and receive NULL/FALSE without having to catch an exception
- Besides, we don't actually need to instanciate the controller here, but merely check that one is defined. So getControllerClass() should have this "NULL / no exception please" ability.

[edit : pinged folks in #1807042: Reorganizie entity storage/list/form/render controller annotation keys about this]

sun’s picture

Status: Needs review » Needs work

Exceptions are for actual error conditions only. We should not throw exceptions for perfectly valid runtime conditions/behavior.

Instead of instantiating the controller, why don't you retrieve the entity info and check whether there's a render controller defined?

yched’s picture

Instead of instantiating the controller, why don't you retrieve the entity info and check whether there's a render controller defined?

Sure, I have a patch that does this - but that is not ideal encapsulation IMO. Dealing with this should be the job of the EntityManager (like, #1807042: Reorganizie entity storage/list/form/render controller annotation keys is about to reshuffle the whole structure of the 'controller classes' within the info).
getControllerClass() *almost* does what we need, we just need a "no exceptions" behavior.

catch’s picture

Why is RDF module even implementing hook_entity_load(), this would make more sense in hook_entity_prepare_view() no?

sun’s picture

+1 to #9. Stellar. :)

yched’s picture

Yup, rdf_entity_load() populates $entity->rdf_mapping, but the data is only actually used in implementations of hook_preprocess_[entity]() or hook_field_attach_view_alter(). So loading it in hook_entity_prepare_view() would make sense.

One exception though : rdf_comment_load() uses the 'rdf_mapping' property set in rdf_entity_load() :

/**
 * Implements hook_comment_load().
 */
function rdf_comment_load($comments) {
  foreach ($comments as $comment) {
    // Pages with many comments can show poor performance. This information
    // isn't needed until rdf_preprocess_comment() is called, but set it here
    // to optimize performance for websites that implement an entity cache.
    $comment->rdf_data['date'] = rdf_rdfa_attributes($comment->rdf_mapping['created'], $comment->created->value);
    $comment->rdf_data['nid_uri'] = url('node/' . $comment->nid->target_id);
    // ...
}

"set it here to optimize performance for websites that implement an entity cache" :-)
Thoughts ?

dawehner’s picture

We maybe have to think whether it actually makes sense to "require" a certain controller, as they should be maybe optional if possible. An exception kind of requires it, and you really don't want to use the plugin definition directly.

What about using the exception and let the client code, check whether (if ($render_controller instanceof EntityRenderControllerInterface))?

yched’s picture

Also, even though rdf_entity_load() will be better suited as hook_entity_prepare_view() (and thus will only fire on the right entities), we still need to stop rdf_entity_bundle_info_alter() from loading stuff for non-rendereable entity types.

catch’s picture

For caching of the RDFa processing we can do #1605290: Enable entity render caching with cache tag support.

scor’s picture

We're refactoring the RDF mappings to be loaded "on demand" at #1869600: Refactor RDF mappings to be inline with the new Entity Field API, so I would not worry about fixing them here. I'd suggest to mark this issue as dup of #1869600: Refactor RDF mappings to be inline with the new Entity Field API.

dawehner’s picture

dcam’s picture

http://drupal.org/node/1427826 contains instructions for updating the issue summary with the summary template.

The summary may need to be updated with information from comments.

scor’s picture

Status: Needs work » Closed (duplicate)