Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

Status: Active » Needs review
FileSize
161.05 KB

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.

Berdir’s picture

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.

Berdir’s picture

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.

swentel’s picture

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

swentel’s picture

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

swentel’s picture

yched’s picture

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.

Berdir’s picture

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:

  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.

yched’s picture

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

Berdir’s picture

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.

Berdir’s picture

Status: Needs work » Needs review
FileSize
177.54 KB

Re-roll.

yched’s picture

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

Tagging, and RTBC if green. Fingers crossed.

Berdir’s picture

yched’s picture

catch’s picture

Title: Remove entity_type argument from field.module functions that receive a single $entity » Change 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.

yched’s picture

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

xjm’s picture

dawehner’s picture

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

Berdir’s picture

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?

yched’s picture

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.

swentel’s picture

Title: Change notice: Remove $entity_type argument from field.module functions that receive a single $entity » Remove $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.