So the conversion from menu_link to entities makes field / entity field query very confused as it tries to load router_path or other properties because it thinks it's a field. Also somehow, we're getting cache misses on comment_body all the time on node 1. Note that this second issue is field api only, so we might want to split this up in another issue once it's confirmed.

On admin/structure

Screen Shot 2013-03-02 at 14.24.13.png

On node/1

Screen Shot 2013-03-02 at 14.24.13.png

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

swentel’s picture

Issue summary: View changes

Add screenshot

chx’s picture

It's not confused at all. ->condition('router_path', $paths, 'NOT IN') in menu.inc causes $field = field_info_field($specifier); to fire in Drupal\field_sql_storage\Entity\Tables::addField trying to figure out whether router_path is a customizable (field API) field or not. Why is this a problem?

swentel’s picture

Oh ok, yeah, didn't look that far. I guess the annoying part is that all fields are going to be loaded into memory in light of the CMI conversion on pages where we don't actually need them. I guess we can/should make field_info_field() smarter then.

yched’s picture

Priority: Normal » Major

So yeah, the current use of field_info_field() within EFQ is a perf problem, really doesn't play well with the FieldInfo cache strategy, and this shows badly with "Field API -> CMI" : #1735118-149: Convert Field API to CMI

What EFQ really does is "is that $name a Field API field ?". Only if yes, it needs the actual $field.
But the 1st question is "does this field exist ?", and for that it should use field_info_field_map().

swentel’s picture

Issue tags: +Field API

tagging

marcingy’s picture

Status: Active » Needs review
FileSize
1.46 KB

Hopefully this is along the right lines. Entity query tests still pass locally.

Status: Needs review » Needs work

The last submitted patch, field-mapping.patch, failed testing.

marcingy’s picture

Status: Needs work » Needs review
FileSize
4.4 KB

New version this adds a lightweight mapper to get fields by id not sure if this is needed by given the context it seems to make sense.

marcingy’s picture

FileSize
4.34 KB

Doxygen was wrong in last patch.

yched’s picture

Thanks for tackling this, @marcingy !

I'm not sure the part about ids is really needed.

The problematic part of the addField() method is loading a field definition (thus hitting config) just to check whether some string is a field name or not (in which case we consider it's an entity property).
I.e. those lines in HEAD :

      else {
        $field = field_info_field($specifier);  // <- This.
       }
       if ($field) {
         // Do something relevant for a Field API field.
       }
       else {
         // Do something relevant for an entity property)
       }

I'd think the case when $specifier is "id:[something]", that is dealt with be the lines just above that snippet, is not ambiguous and "[something]" is then *always* a field ID. Then, no need to check a field map, calling field_info_field_by_id() is fine.

This could use some confirmation from @chx, though, cause this 'id:' syntax doesn't seem to be documented in Drupal\Core\Entity\QueryInterface ?

Other approach : entity properties are defined in code, and field API fields are defined in config. So it should be much faster to check if something is an entity property first, and only then check if it's a field ?

yched’s picture

Title: field_read_fields is called by menu link + cache misses » EFQ calls field_info_field() too liberally

Re-titling.

chx’s picture

The id: is only for purge purposes and so is not documented. The whole field purge needs a new approach...

chx’s picture

FileSize
1.52 KB

I re-read the issue a few times and wrote an entirely new patch, using marcingy's patch as inspiration only.

Berdir’s picture

I can confirm that field_read_field() is only called for actual fields with this patch. Not sure if it should be called at all, I guess direct access to a certain field isn't cached?

Given that, not sure if I can RTBC this patch like that, today is the big day for Field API so swentel will very likely show up during the day and can hopefully answer that question.

yched’s picture

Status: Needs review » Reviewed & tested by the community

#13 looks just fine, and #1735118: Convert Field API to CMI doesn't touch this file at all, so this shouldn't conflict.
Thanks @chx !

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed e0a111a and pushed to 8.x. Thanks!

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.