Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Right now, almost all field API functions still need a separate entity type argument, which is unnecessary in all cases where we just provide a single entity.
Just like with the entity hooks and similar places, we should remove that argument.
This will probably conflict with #1735118: Convert Field API to CMI but @swentel already said that this is fine with him.
Comments
Comment #1
BerdirOk, let's see if I got everything.
Was a bit more than I expected but most of it is trivial replacement stuff.
Comment #3
BerdirHah, I did miss a few, unsurprisingly.
Fixed most of them, let's see what the tests say.
This is ugly:
But type hinted arguments explicitly need to be set to allow NULL and it is called with NULL for the default values. The alternative would be an empty dummy entity but we currently don't have an easy way to create one.
Comment #5
BerdirOk, some more test failures, this should pass most if not all tests...
Had some fun with the cachable tests as those field tests cheated and passed in a test_entity entity but used test_cacheable_entity as $entity_type. Most of the other things were straight forward.
Comment #6
swentel CreditAttribution: swentel commentedI'm fine with this and I don't see any reason why not todo this unless there's a historical reason that I have no idea of. I'll let yched push the button. If ok, add the 'Avoid commit conflicts' here as this might be an annoying the re-roll every time :)
Comment #7
swentel CreditAttribution: swentel commentedActually, it doesn't apply anymore anyway, give me 5 minutes.
Comment #8
swentel CreditAttribution: swentel commentedRe-roll
Comment #9
yched CreditAttribution: yched commentedYAY ! Thanks for the heroïc effort, @Berdir :-)
So, yeah, now is a good time to do this, especially if/when we have a patch that applies and is green :-)
This one might require some more thinking. $entity is optional, but $entity_type is not. Meaning we apparently don't always have an $entity in all the places where we check field_access() (can't check the code right now though).
[edit: removed remark about _field_invoke_get_instances() changing signature, it actually doesn't - my bad]
unwanted newline
------------------
The rest are non blockers, and a fast commit is preferable to reroll hell.
The type hint for the @param $entity is added in most hooks (which is great) but not in all. Not critical, but while we're in there...
There seems to be a lot of calls to $entity->entityType() now in there. I think in those cases we populate an intermediate $entity_type variable. Might be true for other places too, but drawing the line might not be obvious...
28 days to next Drupal core point release.
Comment #10
BerdirThanks for the review!
- hook_field_access(). Yes, I was wondering about the too. The only case where I found that being used was in the Field views plugin:
I'm not sure if there is a use case where field access could possibly be specific to an entity type but not the entity? At least my guess is that the views code only does this because $entity_type is required right now :)
We can exclude that from this patch it if requires more discussion...
Added a $entity_type variable to field_language() and field_sql_storage_field_storage_write(), I think those two had the most calls. Looks big in the interdiff but actually makes the real patch a (tiny bit) smaller :)
Also fixed the empty line in the docblock and the missing type for @param $entity.
Comment #11
yched CreditAttribution: yched commentedYeah, I'd advise leaving field_access() out for now.
Missing hinting on "@param $entity": there were more than one ;-)
hook_field_insert(), hook_field_attach_presave(), hook_field_attach_insert(), hook_field_attach_update(), hook_field_attach_delete(), hook_field_attach_delete_revision(), hook_field_attach_purge().
Again, no blocker, but since testbot seems blocked :-/
Comment #12
BerdirOk, another re-roll.
- type hint added to all @param entity that I could find except entity label callbacks (they still have $entity_type, but that can be removed now, separate issue) and one for hook_field_attach_create_bundle() which should actually be $bundle but that's so not related to this issue that I'm ignoring it.
- field_access changes reverted.. I think. Let's see if I messed something up, it is almost 3am here ;)
Comment #14
BerdirRe-roll.
Comment #15
yched CreditAttribution: yched commentedTagging, and RTBC if green. Fingers crossed.
Comment #16
BerdirYay!
Opened #1879200: Remove uneeded $entity_type argument from entity type callbacks and #1879214: Incorrect @param $entity on hook_field_attach_create_bundle(), the second is more than easy :)
Comment #17
yched CreditAttribution: yched commented#14: remove-entity_type-argument-from-field-api-1874300-14.patch queued for re-testing.
Comment #18
catchLooks great. Committed/pushed to 8.x. Will need a change notice.
Wondered a bit about the multiple entity type ones as well but it'd be cumbersome to pull the entity type out each time so let's leave those at least for now.
Comment #19
yched CreditAttribution: yched commented@catch: yeah, I'm not sure we're consistent about "multiple entities" functions and hooks right now.
The problem if we don't put an explicit $entity_type param in those, is that if $entities == empty array, then you cannot tell $entity_type at all. Edge case, but...
Comment #20
xjmComment #21
dawehnerAdded a change notice, though I'm not sure how much we should document, see http://drupal.org/node/1882428
Comment #22
BerdirHm, I find a shorter before/after example easier to read. And the list of changed hooks and field API functions was easy to extract from the patch... Updated http://drupal.org/node/1882428, what do others think?
Comment #23
yched CreditAttribution: yched commentedThanks again for this @Berdir.
Adjusted the change notice a bit (notably removed the
function
keyworks and list of arguments in the list of affected functions & hooks, more legible IMO.Comment #24
swentel CreditAttribution: swentel commentedResetting some stuff.