Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
On node/1
Comment | File | Size | Author |
---|---|---|---|
#13 | 1931900_13.patch | 1.52 KB | chx |
#9 | field-mapping.patch | 4.34 KB | marcingy |
#8 | field-mapping.patch | 4.4 KB | marcingy |
#6 | field-mapping.patch | 1.46 KB | marcingy |
Screen Shot 2013-03-02 at 14.29.51.png | 43.18 KB | swentel |
Comments
Comment #0.0
swentel CreditAttribution: swentel commentedAdd screenshot
Comment #1
chx CreditAttribution: chx commentedIt's not confused at all.
->condition('router_path', $paths, 'NOT IN')
inmenu.inc
causes$field = field_info_field($specifier);
to fire inDrupal\field_sql_storage\Entity\Tables::addField
trying to figure out whetherrouter_path
is a customizable (field API) field or not. Why is this a problem?Comment #2
swentel CreditAttribution: swentel commentedOh 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.
Comment #3
yched CreditAttribution: yched commentedSo 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().
Comment #5
swentel CreditAttribution: swentel commentedtagging
Comment #6
marcingy CreditAttribution: marcingy commentedHopefully this is along the right lines. Entity query tests still pass locally.
Comment #8
marcingy CreditAttribution: marcingy commentedNew 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.
Comment #9
marcingy CreditAttribution: marcingy commentedDoxygen was wrong in last patch.
Comment #10
yched CreditAttribution: yched commentedThanks 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 :
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 ?
Comment #11
yched CreditAttribution: yched commentedRe-titling.
Comment #12
chx CreditAttribution: chx commentedThe id: is only for purge purposes and so is not documented. The whole field purge needs a new approach...
Comment #13
chx CreditAttribution: chx commentedI re-read the issue a few times and wrote an entirely new patch, using marcingy's patch as inspiration only.
Comment #14
BerdirI 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.
Comment #15
yched CreditAttribution: yched commented#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 !
Comment #16
alexpottCommitted e0a111a and pushed to 8.x. Thanks!
Comment #17.0
(not verified) CreditAttribution: commentedUpdated issue summary.