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.
Follow up from #1735118: Convert Field API to CMI
Changes to happen
Talked this through a little with alexpott on IRC and moving the logic to the storage controllers is a good idea. He suggested making the methods also static and renaming the method to findByProperties() but that's more of a config overall DX issue that needs to created.
So for now, just moving the logic and not yet deprecating the old functions is enough for this patch.
Old summary
Move to EFQ ? are they even still needed ?.
Probably involves a separate API to retrieve deleted fields, which very little code actually needs to deal with.
Comment | File | Size | Author |
---|---|---|---|
#23 | field-read-x-1953414-23.patch | 17.62 KB | swentel |
#23 | interdiff.txt | 1.09 KB | swentel |
#22 | interdiff-19vs22.patch | 7.03 KB | andypost |
#22 | field-read-x-1953414-19.patch | 17.62 KB | andypost |
#19 | interdiff.txt | 1.25 KB | andypost |
Comments
Comment #1
swentel CreditAttribution: swentel commentedTagging
Comment #2
yched CreditAttribution: yched commentedUn-postponing.
Comment #3
andypostSuppose it's much easy to mark them as deprecated in favor of
entity_load_multiple_by_properties()
Comment #4
swentel CreditAttribution: swentel commentedLet's see what happens when moving this to the storage controllers - marked the functions deprecated already too as they are still there.
Comment #5
swentel CreditAttribution: swentel commentedTalked this through a little with alexpott on IRC and moving the logic to the storage controllers is a good idea. He suggested making the methods also static and renaming the method to findByProperties() but that's more of a config overall DX issue that needs to created.
So for now, just moving the logic and not yet deprecating the old functions is enough for this patch.
Comment #6
alexpottYep that sums up the discussion nicely.
Comment #8
swentel CreditAttribution: swentel commentedThis should be a little bit better :)
Comment #10
swentel CreditAttribution: swentel commented#8: field-read-x-1953414-8.patch queued for re-testing.
Comment #11
andypostWhy not mark them @deprecated?
Parent implementation already have this method... so probably could be inherited
ControllerInterface should inject em
Comment #12
andypostSo just override contructor
Comment #13
andyposta small copy/paste fix
Comment #14
dawehnerWhy not simply call parent::__construct($entity_type, ...); as it will set all this values as well.
Here as well.
In general I'm wondering whether the really similar logic in both FieldStoragecontroller and FieldInstanceStorageController could be moved to a helper object?
Comment #15
andypoststate should be injected too //cc Crel
Comment #17
andypost@dawehner nice catch! I think a Base class is out of scope but makes sense and leads to bikesheding
Let's address @deprecated in other issue
Comment #19
andypostShould be fine now
Comment #20
Crell CreditAttribution: Crell commentedThis should be injected, too.
Comment #21
dawehnerWhy not use Drupal::entityManager()->getStorageController('field_entity')->loadByProperties($conditions) directly?
There should be always the ModuleHandlerInterface type hinted as it exists.
So the storage controller is used that often ... why not store it in the constructor on the object.
Comment #22
andypostLet's leave
entity_load_multiple_by_properties()
as is (procedural wrapper)Other fixed
Comment #23
swentel CreditAttribution: swentel commentedGood to go, two extreme nitpicks in the patch.
Comment #24
andypost+1 to rtbc
Comment #25
alexpottCommitted e416a5c and pushed to 8.x. Thanks!
Comment #26
swentel CreditAttribution: swentel commentedSweet - @yched, now drop field_read_field/instance - field_read_fields/instances completely and depend on entity_load_multiple_by_properties() ?
- edit - although that's against my comment earlier of course.
- edit 2 - probably just keep them for now - alex suggested that too
Comment #27
yched CreditAttribution: yched commented@swentel: hmm, I'm a bit lost here actually. Can you summarize the options ?
Comment #28
swentel CreditAttribution: swentel commentedWell, just like we're killing field_create_field etc, kill field_read_field - field_read_fields() - field_read_instance() - field_read_instances().
People will instead have todo something like this:
We have some special keys for $conditions, like 'include_inactive' and 'include_deleted' which don't actually map to properties and are now handled within field_read_fields() and field_read_instances(). This might make it a bit more confusing.
Also, a lot of search and replaces of course in core if we do this. Not sure, it feels consistent, but I have no strong opinion.
Comment #29
yched CreditAttribution: yched commentedIdeally, yes, we'd want to get rid of field_read_*() IMO.
- The notion of inactive fields should theoretically go away with "modules can't be disabled anymore"
- I think I remember a discussion in Portland where there was an agreement to remove the support for querying deleted & non-deleted fields in the same API function. Like "if you need a deleted field / instance, fetch it yourself from state()".
Or maybe it was in the issue queue. I do remember a heavy "yes" from @alexpott on this point :-)
Comment #30
swentel CreditAttribution: swentel commentedAlright, filed #2018319: Remove field_read_field(s)() and field_read_instance(s) in favor of entity_load() and entity_load_multiple_by_properties().
Comment #31.0
(not verified) CreditAttribution: commentedUpdated issue summary.