Remove $type from entity hooks

Can we merge this with #1618154: add type-hinting for $entity parameter ?

Patch for this and #1618154: add type-hinting for $entity parameter

Thnx for starting this but this needs extra work

+++ b/core/modules/entity/entity.api.phpundefined
@@ -242,10 +240,8 @@ function hook_entity_load($entities, $type) {
  * @param $entity
  *   The entity object.
- * @param $type
- *   The type of entity being saved (i.e. node, user, comment).
  */
-function hook_entity_presave($entity, $type) {
+function hook_entity_presave(EntityInterface $entity) {

In .api.php files we need to declare te full path. EntiInterface should be Drupal\entity\EntityInterface. And we need to define the type after @param. SO that should be: @param Drupal\entity\EntityInterface $entity

Second of all you should grep for "Implement_hook_entity_*" in drupal 8. Replace * with one of the hooks.
You'll find one instance for all in a test file. There you need to add the typehint.

In those classes you can add "use Drupal\entity\EntityInterface" on top of the file and than you can use EntityInterface to typehint.

I hope this makes a bit sense... ;)

14 days to next Drupal core point release.

Rerolled accordingly.

+++ b/core/modules/entity/tests/modules/entity_crud_hook_test/entity_crud_hook_test.moduleundefined
@@ -5,10 +5,12 @@
+function entity_crud_hook_test_entity_presave(EntityInterface $entity, $type) {

We need to delete the $type if I'm correct :)

Same for the other functions

14 days to next Drupal core point release.

Previous patch was still missing a few changes.

@aspilicious: indeed, just rerolled for that reason :-)

+++ b/core/modules/entity/entity.api.phpundefined
@@ -327,17 +317,15 @@ function hook_entity_predelete($entity, $type) {
-  $info = entity_get_info($type);
-  list($id) = entity_extract_ids($type, $entity);
+  $info = entity_get_info($entity->entityType());
+  list($id) = entity_extract_ids($entity);
   db_delete('example_entity')

entity_extract id's is wrong is having incorrect paramaters. But we don't need that here anymore we just can call $entity->id() (or something like that, just check the entity.php class)

+++ b/core/modules/entity/entity.api.phpundefined
@@ -228,42 +228,36 @@ function hook_entity_info_alter(&$entity_info) {
+  list($id) = entity_extract_ids($entity);

same

+++ b/core/modules/entity/entity.api.phpundefined
@@ -274,20 +268,18 @@ function hook_entity_insert($entity, $type) {
+  list($id) = entity_extract_ids($entity);

Same

+++ b/core/modules/entity/entity.api.phpundefined
@@ -297,17 +289,15 @@ function hook_entity_update($entity, $type) {
+  list($id) = entity_extract_ids($entity);

Same

14 days to next Drupal core point release.

And the info stuff looks useless too...

Not exactly the same issue, but close enough to include it.

+++ b/core/modules/entity/entity.api.phpundefined
@@ -440,12 +419,10 @@ function hook_entity_view_alter(&$build, $type) {
+function hook_entity_prepare_view($entities) {
   // Load a specific node into the user object for later theming.
-  if ($type == 'user') {
+  if ($entity->entityType() == 'user') {

mmm we have an entity array and we are fetching the type from it... We have to check if it's not empty and use the first entity than... (or just use the first entity as it just an api doc)

14 days to next Drupal core point release.

Good catch. This should be better.

I also added the full signature for Drupal\entity\EntityFieldQuery on hook_entity_query_alter() while changing that file.

+++ b/core/modules/entity/entity.api.phpundefined
@@ -440,12 +419,10 @@ function hook_entity_view_alter(&$build, $type) {
+  if (!empty($entities) && reset($entities)->type() == 'user') {

entityType

14 days to next Drupal core point release.

Can't even guess why I didn't change that one.

Yeah I agree with the above actually...

The view alter stuff looks like a duplicate of #1618140: hook_entity_view_alter() misses an $entity parameter, maybe just leave this out in this issue?

Rerolled included #1618140: hook_entity_view_alter() misses an $entity parameter and the changes from #16. Tests for comment, entity, node, taxonomy, and user pass locally.

#19: 0001-Issue-1618136-remove-type-and-entity_extract_ids-fro.patch queued for re-testing.

(these 2 errors just do not make sense: they happen outside the test)

Ouch as you say. Rerolled and credit added.

Is rtbc IMO.

Rerolled after yesternight's commits, just in case they broke something. Setting to CNR just for bot to check.

I think this is conceptually blocked by #1551140: Remove stub entities, replace entity_create_stub_entity(). During the delete operation, we currently pass a stub of the entity to field hooks. That's not going to work anymore if the stub entities are not classed themselves.

@DamZ: just discussed this with Berdir, and he's saying that since #1551140: Remove stub entities, replace entity_create_stub_entity() only affects the field hook, and this only affects the entity CRUD hooks, it shouldn't be a blocker.

Talking with @berdir, we are not actually blocked here, because we load the entity everywhere, including in delete.

Looking at the patch, it seems to be missing updates to the documentation:

-  drupal_alter(array('taxonomy_term_view', 'entity_view'), $build, $type);
+  drupal_alter(array('taxonomy_term_view', 'entity_view'), $build, $term);

We modify the entity-type specific hook signature without updating its API documentation (true for all entity types in core).

Rerolled accordingly.

resetting to rtbc as per #24.

And here they are.

Cannot guess how this even happens.

Someday we'll get rid of global $user

Prospective change notice created at http://drupal.org/node/1637540

So we had a discussion about this:

-function hook_entity_presave($entity, $type) {
+function hook_entity_presave(Drupal\entity\EntityInterface $entity) {

That looks bloody awful from a "newish to Drupal 8" perspective, but OTOH this is the current standard, so it's correct. The reason we do that is so you can copy/paste these implementations into your module without having to also figure out where the class comes from and toss the namespace at the top of your file. However, not the fault of this patch, and I'm actually reasonably sure I was one of the people advocating for this originally so.. ;P

Committed and pushed to 8.x. Thanks! :)

Also reviewed the change notice and it looks great. :)

+++ b/core/modules/node/node.module
@@ -1159,7 +1159,7 @@ function node_view(Node $node, $view_mode = 'full', $langcode = NULL) {
   $type = 'node';
-  drupal_alter(array('node_view', 'entity_view'), $build, $type);
+  drupal_alter(array('node_view', 'entity_view'), $build, $node);

Stray $type variable, which should be removed.

Ow we forgot to remove

$type = 'node';

Yup.

Simple enough, RTBC unless the testbot disagrees, and I don't think he will ;)

Committed and pushed to 8.x. Thanks for the catch. :)