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.
Noticed while reviewing #2198429: Make deleted fields work with config synch :
When doing entity_load_multiple_by_properties('field_config', array('deleted' => TRUE, 'include_deleted' => TRUE)) (which is the recommended way to get "deleted fields pending purging"), FieldConfigStorage::loadByProperties() starts by loading all field_config entities.
That's useless if we have 'deleted' => TRUE in the conditions.
Comment | File | Size | Author |
---|---|---|---|
#21 | interdiff.txt | 1.63 KB | yched |
#21 | FieldConfig-load_by_properties-2247095-21.patch | 4.03 KB | yched |
#18 | FieldConfig-load_by_properties-2247095-18.patch | 4 KB | yched |
#17 | FieldConfig-load_by_properties-2247095-17.patch | 3.43 KB | yched |
#11 | FieldConfig-load_by_properties-2247095-11.patch | 2.13 KB | yched |
Comments
Comment #1
yched CreditAttribution: yched commentedPatch
Comment #2
xjmI started trying to improve this comment to clarifying what was going on (in addition to getting rid of the trailing whitespace), but it wasn't clear to me that any field data would necessarily be loaded when
$conditions['deleted']
was set. Do you have to pass both thedeleted
andinclude_deleted
conditions to get deleted fields? So we are relying on that, otherwise this returns nothing?Presumably if the calling code is looking for only deleted fields, it wants to include them? Is that a safe assumption? That's what patch A shows; it's more a question about the intended functionality than anything. Patch B is just the comment improvement.
Comment #4
alexpott2a makes sense to me if you've set
$conditions['deleted']
to TRUE then having to set the include deleted seems ucky.Comment #5
yched CreditAttribution: yched commentedYeah, currently if you specify 'deleted' = TRUE *without* 'include_deleted' = TRUE, you get nothing. That's the case in HEAD.
Might be changed to make 'include_deleted' implicit if 'deleted' = TRUE, sure, but strictly speaking that's outside the scope of the optimiztion here, and I didn't really want to derail on discussions whether the above is a good idea or not :-). Either way works for me.
Comment #6
xjmThanks @alexpott for fixing my syntax error... :)
@yched, where is the documentation for the
include_deleted
faux condition in the codebase?Comment #7
yched CreditAttribution: yched commentedHmm, nowhere at the moment, it seems :-/.
AFAICT, when the logic got moved to FieldConfigStorage::loadByProperties() the doc remained in the phpdoc for field_read_fields(), and then got lost when field_read_fields() got removed...
Comment #8
yched CreditAttribution: yched commented4: 2247095.4.patch queued for re-testing.
Comment #9
yched CreditAttribution: yched commentedBump - still worth doing :-)
Comment #11
yched CreditAttribution: yched commentedHah, reroll for PSR-4, of course :-)
Comment #12
xjmLet's file a separate issue (or two) for #5-#7 (re-documenting the
deleted
faux-condition and considering what to do about it andinclude_deleted
). The reason I asked where it was documented is that I wanted to add a reference to it here and use it to explain the logic, but if it's nowhere, that's a separate bug and we can update the docs added by this patch as a part of that docs fix.Comment #13
BerdirStupid question but if we optimize it to only load from deleted if include_deleted is TRUE (which would be misnamed then?), why do we bother to fake a loadByProperties() load anyway, we could just move this to a standalone function/method?
Comment #14
xjmSeparate method sounds nice.
Comment #15
yched CreditAttribution: yched commentedNot sure I get #13.
The optimizations are :
- do not include fields from the "state() liste of deleted fields" if $conditions['include_deleted'] != TRUE - that's the semantics of 'include_deleted'
- do not include fields form the regular config store if $conditions['deleted'] == TRUE (since we know in advance they will never meet this condition)
But the current method still lets you do stuff like ('type' => 'image', 'include_deleted' => TRUE), that will find all image fields, deleted or not.
Comment #16
yched CreditAttribution: yched commentedOpened #2285427: document the 'include_deleted' faux condition in Field(Instance)ConfigStorage::loadByProperties
Comment #17
yched CreditAttribution: yched commentedNeeded a reroll after #2277941: Allow injecting an arbitrary FieldConfig when building a FieldInstanceConfig, + no reason not to do the same in FieldInstanceConfigStorage
Comment #18
yched CreditAttribution: yched commentedCrap, #17 forgot a hunk.
Comment #21
yched CreditAttribution: yched commentedOh gee, doing stuff by chunks of "5 minutes available time slot" produces nothing good...
- my latest patch was borked (&& instead of ||...).
- Forgot the earlier course of the discussion here.
My reply in #15 was based on what my original patch #1 was doing - merely adding an optimization while keeping the behavior unchanged.
Then #2 / #4 added "look at deleted fields if either 'include_deleted' *or 'deleted'* are TRUE (i.e : specifying a "deleted = TRUE" condition implicitly triggers include_deleted behavior). Attached patch keeps that part.
The conclusion of my reply #15 still stands though : the current logic lets you find the fields that match some condition and are either deleted or non-deleted. Yiou can't do that anymore if we move "querying deleted fields" to a separate method.
Sorry for the confusion :-/
Comment #22
BerdirOk, I get it now, makes sense, ignore what I wrote.
I was looking if there are any calls that we should update to use 'deleted' instead of include_deleted, but looks like the case in FieldInstanceConfig is now gone now that we inject the field there.
The only other places is Tables.php which could use this but that's AFAIK going away anyway in #2144263: Decouple entity field storage from configurable fields.
Comment #23
xjmComment #24
catchThere was a trailing whitespace issue in the patch that I fixed on commit. Committed/pushed to 8.x, thanks!