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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yched’s picture

Status: Active » Needs review
FileSize
1.8 KB

Patch

xjm’s picture

+++ b/core/modules/field/lib/Drupal/field/FieldConfigStorage.php
@@ -97,15 +97,20 @@ public function loadByProperties(array $conditions = array()) {
+    ¶
+    // Get fields stored in configuration (not needed when looking for deleted
+    // fields explicitly).

I 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 the deleted and include_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.

The last submitted patch, 2: field-2247095-2A.patch, failed testing.

alexpott’s picture

FileSize
1.21 KB
2.18 KB

2a makes sense to me if you've set $conditions['deleted'] to TRUE then having to set the include deleted seems ucky.

yched’s picture

Yeah, 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.

xjm’s picture

Thanks @alexpott for fixing my syntax error... :)

@yched, where is the documentation for the include_deleted faux condition in the codebase?

yched’s picture

where is the documentation for the include_deleted faux condition in the codebase ?

Hmm, 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...

yched’s picture

4: 2247095.4.patch queued for re-testing.

yched’s picture

Bump - still worth doing :-)

Status: Needs review » Needs work

The last submitted patch, 4: 2247095.4.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
2.13 KB

Hah, reroll for PSR-4, of course :-)

xjm’s picture

Let's file a separate issue (or two) for #5-#7 (re-documenting the deleted faux-condition and considering what to do about it and include_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.

Berdir’s picture

Stupid 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?

xjm’s picture

Separate method sounds nice.

yched’s picture

Not 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.

yched’s picture

yched’s picture

Needed a reroll after #2277941: Allow injecting an arbitrary FieldConfig when building a FieldInstanceConfig, + no reason not to do the same in FieldInstanceConfigStorage

yched’s picture

Crap, #17 forgot a hunk.

The last submitted patch, 17: FieldConfig-load_by_properties-2247095-17.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 18: FieldConfig-load_by_properties-2247095-18.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
4.03 KB
1.63 KB

Oh 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 :-/

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Ok, 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.

xjm’s picture

catch’s picture

Status: Reviewed & tested by the community » Fixed

There was a trailing whitespace issue in the patch that I fixed on commit. Committed/pushed to 8.x, thanks!

  • catch committed fee42cb on 8.x
    Issue #2247095 by yched, xjm, alexpott: Optimize loading of deleted...

Status: Fixed » Closed (fixed)

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