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.

Files: 
CommentFileSizeAuthor
#14 remove-entity_type-argument-from-field-api-1874300-14.patch177.54 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 50,477 pass(es).
[ View ]
#12 remove-entity_type-argument-from-field-api-1874300-12.patch178.43 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch remove-entity_type-argument-from-field-api-1874300-12.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#12 remove-entity_type-argument-from-field-api-1874300-12-interdiff.txt15.65 KBBerdir
#10 remove-entity_type-argument-from-field-api-1874300-10.patch176.91 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch remove-entity_type-argument-from-field-api-1874300-10.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#10 remove-entity_type-argument-from-field-api-1874300-10-interdiff.txt5.62 KBBerdir
#8 remove-entity_type-argument-from-field-api-1874300-8.patch179.35 KBswentel
PASSED: [[SimpleTest]]: [MySQL] 50,099 pass(es).
[ View ]
#5 remove-entity_type-argument-from-field-api-1874300-5.patch179.33 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 50,006 pass(es).
[ View ]
#5 remove-entity_type-argument-from-field-api-1874300-5-interdiff.txt12.36 KBBerdir
#3 remove-entity_type-argument-from-field-api-1874300-3.patch169 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 49,813 pass(es), 114 fail(s), and 541 exception(s).
[ View ]
#3 remove-entity_type-argument-from-field-api-1874300-3-interdiff.txt17.35 KBBerdir
#1 remove-entity_type-argument-from-field-api-1874300-1.patch161.05 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 49,316 pass(es), 379 fail(s), and 1,351 exception(s).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new161.05 KB
FAILED: [[SimpleTest]]: [MySQL] 49,316 pass(es), 379 fail(s), and 1,351 exception(s).
[ View ]

Ok, let's see if I got everything.

Was a bit more than I expected but most of it is trivial replacement stuff.

Status:Needs review» Needs work

The last submitted patch, remove-entity_type-argument-from-field-api-1874300-1.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new17.35 KB
new169 KB
FAILED: [[SimpleTest]]: [MySQL] 49,813 pass(es), 114 fail(s), and 541 exception(s).
[ View ]

Hah, I did miss a few, unsurprisingly.

Fixed most of them, let's see what the tests say.

This is ugly:

-function hook_field_validate(\Drupal\Core\Entity\EntityInterface $entity, $field, $instance, $langcode, $items, &$errors) {
+function hook_field_validate(\Drupal\Core\Entity\EntityInterface $entity = NULL, $field, $instance, $langcode, $items, &$errors) {

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.

Status:Needs review» Needs work

The last submitted patch, remove-entity_type-argument-from-field-api-1874300-3.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new12.36 KB
new179.33 KB
PASSED: [[SimpleTest]]: [MySQL] 50,006 pass(es).
[ View ]

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

Assigned:Unassigned» yched

I'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 :)

Actually, it doesn't apply anymore anyway, give me 5 minutes.

StatusFileSize
new179.35 KB
PASSED: [[SimpleTest]]: [MySQL] 50,099 pass(es).
[ View ]

Re-roll

Status:Needs review» Needs work

YAY ! 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 :-)

+++ b/core/modules/field/field.api.phpundefined
@@ -2250,7 +2192,7 @@ function hook_field_storage_purge($entity_type, $entity, $field, $instance) {
  * @return
  *   TRUE if the operation is allowed, and FALSE if the operation is denied.
  */
-function hook_field_access($op, $field, $entity_type, $entity, $account) {
+function hook_field_access($op, $field, \Drupal\Core\Entity\EntityInterface $entity, $account) {
   if ($field['field_name'] == 'field_of_interest' && $op == 'edit') {
     return user_access('edit field of interest', $account);

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]

+++ b/core/modules/field/field.api.phpundefined
@@ -1554,20 +1510,18 @@ function hook_field_storage_write($entity_type, $entity, $op, $fields) {
- *
- * @param $entity_type
- *   The entity type of entity, such as 'node' or 'user'.
- * @param $entity
+
+ * @param \Drupal\Core\Entity\EntityInterface $entity

unwanted newline

------------------

The rest are non blockers, and a fast commit is preferable to reroll hell.

+++ b/core/modules/field/field.api.phpundefined
@@ -982,12 +956,10 @@ function hook_field_attach_submit($entity_type, $entity, $form, &$form_state) {
- * @param $entity_type
- *   The type of $entity; e.g. 'node' or 'user'.
  * @param $entity
  *   the entity with fields to process.
  */
-function hook_field_attach_presave($entity_type, $entity) {

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

+++ b/core/modules/field/field.multilingual.incundefined
@@ -299,18 +299,18 @@ function field_valid_language($langcode, $default = TRUE) {
-function field_language($entity_type, $entity, $field_name = NULL, $langcode = NULL) {

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.

Status:Needs work» Needs review
StatusFileSize
new5.62 KB
new176.91 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch remove-entity_type-argument-from-field-api-1874300-10.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Thanks 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:

<?php
 
public function access() {
    
$base_table = $this->get_base_table();
-    return
field_access('view', $this->field_info, $this->definition['entity_tables'][$base_table]);
+    return
field_access('view', $this->field_info);
   }
?>

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.

Yeah, 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 :-/

StatusFileSize
new15.65 KB
new178.43 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch remove-entity_type-argument-from-field-api-1874300-12.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Ok, 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 ;)

Status:Needs review» Needs work

The last submitted patch, remove-entity_type-argument-from-field-api-1874300-12.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new177.54 KB
PASSED: [[SimpleTest]]: [MySQL] 50,477 pass(es).
[ View ]

Re-roll.

Status:Needs review» Reviewed & tested by the community
Issue tags:+Avoid commit conflicts

Tagging, and RTBC if green. Fingers crossed.

Title:Remove entity_type argument from field.module functions that receive a single $entityChange notice: Remove $entity_type argument from field.module functions that receive a single $entity
Priority:Normal» Critical
Status:Reviewed & tested by the community» Active

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

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

Added a change notice, though I'm not sure how much we should document, see http://drupal.org/node/1882428

Status:Active» Needs review

Hm, 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?

Status:Needs review» Fixed

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

Title:Change notice: Remove $entity_type argument from field.module functions that receive a single $entityRemove $entity_type argument from field.module functions that receive a single $entity
Priority:Critical» Normal
Issue tags:-Needs change record

Resetting some stuff.

Status:Fixed» Closed (fixed)

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