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

Only local images are allowed.On node/1

Only local images are allowed.

Files: 
CommentFileSizeAuthor
#13 1931900_13.patch1.52 KBchx
PASSED: [[SimpleTest]]: [MySQL] 54,322 pass(es).
[ View ]
#9 field-mapping.patch4.34 KBmarcingy
PASSED: [[SimpleTest]]: [MySQL] 54,531 pass(es).
[ View ]
#8 field-mapping.patch4.4 KBmarcingy
PASSED: [[SimpleTest]]: [MySQL] 54,519 pass(es).
[ View ]
#6 field-mapping.patch1.46 KBmarcingy
FAILED: [[SimpleTest]]: [MySQL] 54,239 pass(es), 38 fail(s), and 3 exception(s).
[ View ]
Screen Shot 2013-03-02 at 14.29.51.png43.18 KBswentel
Screen Shot 2013-03-02 at 14.24.13.png60.31 KBswentel

Comments

Issue summary:View changes

Add screenshot

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?

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.

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().

Issue tags:+Field API

tagging

Status:Active» Needs review
StatusFileSize
new1.46 KB
FAILED: [[SimpleTest]]: [MySQL] 54,239 pass(es), 38 fail(s), and 3 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new4.4 KB
PASSED: [[SimpleTest]]: [MySQL] 54,519 pass(es).
[ View ]

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.

StatusFileSize
new4.34 KB
PASSED: [[SimpleTest]]: [MySQL] 54,531 pass(es).
[ View ]

Doxygen was wrong in last patch.

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 ?

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

Re-titling.

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

StatusFileSize
new1.52 KB
PASSED: [[SimpleTest]]: [MySQL] 54,322 pass(es).
[ View ]

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

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.

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 !

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.

Issue summary:View changes

Updated issue summary.