Problem/Motivation

The entity_view</code and <code>entity_view_multiple functions have been deprecated for some time now. They should not be used in core as they are deprecated.

Proposed resolution

Remove the final usages from core and trigger a deprecation warning when the functions are called by implementing modules.

Remaining tasks

review
commit

User interface changes

none

API changes

none

Data model changes

Release notes snippet

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

legolasbo created an issue. See original summary.

legolasbo’s picture

Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, remove-remaining-usages-of-entity-view-multiple.patch, failed testing. View results

legolasbo’s picture

Status: Needs work » Needs review

The test failed due to the following error, which seems unrelated to me:

PHP Warning:  file_put_contents(/var/lib/drupalci/workspace/jenkins-drupal_patches-59007/source/core/.env): failed to open stream: Permission denied in /opt/drupalci/testrunner/src/DrupalCI/Plugin/BuildTask/BuildStep/Testing/NightwatchJS.php on line 62

Status: Needs review » Needs work

The last submitted patch, remove-remaining-usages-of-entity-view-multiple.patch, failed testing. View results

legolasbo’s picture

Status: Needs work » Needs review

Back to needs review now the testbot issues have been resolved.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

amateescu’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +@deprecated

Looks good to me.

catch’s picture

+++ b/core/modules/node/node.module
@@ -828,7 +828,9 @@ function node_view(NodeInterface $node, $view_mode = 'full', $langcode = NULL) {
 function node_view_multiple($nodes, $view_mode = 'teaser', $langcode = NULL) {

Is there already an issue open to properly deprecate node_view_multiple()?

legolasbo’s picture

I don't think there is @catch, and the same seems to be true for comment_view_multiple(), user_view_multiple() and taxonomy_term_view_multiple(). Those issues should be created, but that shouldn't block this patch from landing should it?

larowlan’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/includes/entity.inc
@@ -403,6 +403,9 @@ function entity_view(EntityInterface $entity, $view_mode, $langcode = NULL, $res
+  $error_msg = __METHOD__ . ' is deprecated as of Drupal 8.0.x and will be removed before Drupal 9.0.0. Instead, use \Drupal::entityTypeManager()->getViewBuilder($entity->getEntityTypeId())->viewMultiple($entities, $view_mode, $langcode);';
+  trigger_error($error_msg, E_USER_DEPRECATED);

We need a test for the deprecation error being triggered

voleger’s picture

andypost’s picture

Status: Needs work » Needs review
FileSize
31.41 KB
35.76 KB

Added CR https://www.drupal.org/node/3033656 which explains deprecation and provides examples
There was no CR for already deprecated methods

Meantime I think we can deprecate node, term and user same time with #2974258: Remove remaining usages of entity_view

Patch fixes message & deprecate remains so I think we can close second issue

andypost’s picture

Issue needs better title as it have commutative patch, also CR needs review

Berdir’s picture

Status: Needs review » Needs work

+1 to explicitly deprecate all calling functions here as well, we have to touch them anyway. Size of the patch seems managable.

  1. +++ b/core/includes/entity.inc
    @@ -352,10 +352,12 @@ function entity_page_label(EntityInterface $entity, $langcode = NULL) {
      */
     function entity_view(EntityInterface $entity, $view_mode, $langcode = NULL, $reset = FALSE) {
    +  @trigger_error('entity_view() is deprecated in Drupal 8.0.x and will be removed before Drupal 9.0.0. Use \Drupal::entityTypeManager()->getViewBuilder($entity->getEntityTypeId())->view($entity, $view_mode, $langcode); instead. See https://www.drupal.org/node/3033656', E_USER_DEPRECATED);
       $render_controller = \Drupal::entityManager()->getViewBuilder($entity->getEntityTypeId());
       if ($reset) {
    

    should be Druapl 8.0.0 in all those deprecation messages. I'm also still not sure if it should be drupal:8.0.0 now or "Drupal 8.0.0", the format that was proposed in the issue where that is discussed I think proposes the first, but that is not really used anywhere yet.

  2. +++ b/core/modules/comment/comment.module
    @@ -272,10 +272,15 @@ function comment_node_view_alter(array &$build, EntityInterface $node, EntityVie
      */
     function comment_view(CommentInterface $comment, $view_mode = 'full', $langcode = NULL) {
    -  return entity_view($comment, $view_mode, $langcode);
    +  @trigger_error("comment_view() is deprecated in Drupal 8.0.x and will be removed before Drupal 9.0.0. Use \Drupal::entityTypeManager()->getViewBuilder('comment')->view(); instead. See https://www.drupal.org/node/3033656", E_USER_DEPRECATED);
    +  return \Drupal::entityTypeManager()
    +    ->getViewBuilder('comment')
    +    ->view($comment, $view_mode, $langcode);
     }
    

    seeems a bit pointless to change the old function, but I guess that's because we otherwise get two deprecation messages. doesn't really matter though.

  3. +++ b/core/modules/comment/src/Plugin/views/field/EntityLink.php
    @@ -61,7 +61,11 @@ public function preRender(&$values) {
    -      $this->build = entity_view_multiple($entities, $this->options['teaser'] ? 'teaser' : 'full');
    +      $entityTypeId = reset($entities)->getEntityTypeId();
    +      $viewMode = $this->options['teaser'] ? 'teaser' : 'full';
    +      $this->build = \Drupal::entityTypeManager()
    +        ->getViewBuilder($entityTypeId)
    +        ->viewMultiple($entities, $viewMode);
         }
    

    maybe this already has the entity type manager injected? not sure if we should add it here if not.

  4. +++ b/core/modules/contact/contact.module
    @@ -140,7 +140,9 @@ function contact_mail($key, &$message, $params) {
    -      $build = entity_view($contact_message, 'mail');
    +      $build = \Drupal::entityTypeManager()
    +        ->getViewBuilder($contact_message->getEntityTypeId())
    +        ->view($contact_message, 'mail');
    

    we know it is contact_message, so not necessary to make it pseudo-generic with that $entity->getEntityTypeId()?

  5. +++ b/core/modules/field_ui/tests/src/Functional/ManageDisplayTest.php
    @@ -242,7 +242,9 @@ public function assertNodeViewTextHelper(EntityInterface $node, $view_mode, $tex
         // Render a cloned node, so that we do not alter the original.
         $clone = clone $node;
    -    $element = node_view($clone, $view_mode);
    +    $element = \Drupal::entityTypeManager()
    +      ->getViewBuilder($clone->getEntityTypeId())
    +      ->view($clone, $view_mode);
    

    this also knows it is a node..

  6. +++ b/core/modules/file/tests/src/Kernel/FileItemTest.php
    @@ -141,7 +141,9 @@ public function testFileItem() {
         $uri = $file3->getFileUri();
    -    $output = entity_view($entity, 'default');
    +    $output = \Drupal::entityTypeManager()
    +      ->getViewBuilder($entity->getEntityTypeId())
    +      ->view($entity, 'default');
    

    and this probably too, since it is a test.

  7. +++ b/core/modules/node/node.module
    @@ -644,7 +644,9 @@ function template_preprocess_node(&$variables) {
           // not be configured, so remember to always check the theme setting first.
    -      $variables['author_picture'] = user_view($node->getOwner(), 'compact');
    +      $variables['author_picture'] = \Drupal::entityTypeManager()
    +        ->getViewBuilder('user')
    +        ->view($node->getOwner(), 'compact');
    

    this will conflict with [#/2923701] I guess.

  8. +++ b/core/modules/node/node.module
    @@ -815,9 +817,17 @@ function node_get_recent($number = 10) {
      */
     function node_view(NodeInterface $node, $view_mode = 'full', $langcode = NULL) {
    -  return entity_view($node, $view_mode, $langcode);
    +  @trigger_error("node_view() is deprecated in Drupal 8.7.x and will be removed before Drupal 9.0.0. Use \Drupal::entityTypeManager()->getViewBuilder('node')->view(); instead. See https://www.drupal.org/node/3033656", E_USER_DEPRECATED);
    +  return \Drupal::entityTypeManager()
    +    ->getViewBuilder('node')
    +    ->view($node, $view_mode, $langcode);
    

    Not sure which version we should use as being deprecated here. IMHO the main information of that is which version is safe to rely on. This sounds like the replacement is only available in 8.7, which is not true, but not sure if we have any explicit guidelines on that.

  9. +++ b/core/modules/node/src/Plugin/views/row/Rss.php
    @@ -126,7 +126,9 @@ public function render($row) {
     
    -    $build = node_view($node, $build_mode);
    +    $build = \Drupal::entityTypeManager()
    +      ->getViewBuilder('node')
    

    here too, row plugins might have the entity type manager already?

  10. +++ b/core/modules/system/tests/src/Kernel/Entity/EntityViewLegacyTest.php
    @@ -0,0 +1,229 @@
    +
    +/**
    + * Tests entity deprecated functions.
    + *
    + * @group legacy
    + */
    +class EntityViewLegacyTest extends KernelTestBase {
    

    didn't review this up close yet, but the load and delete deprections so far put functions into module specific tests, we could possibly extend those (e.g. they do create load and sometimes delete, so we could put a view in the middle)

andypost’s picture

Fix for #15
1) let's use new format from #3024461: Adopt consistent deprecation format for core and contrib deprecation messages
2) I did convert it because otherwise deprecation will be thrown twice
3) sadly no, probably needs follow-up because it using renderer which already in base class
4-6) fixed
7) nice spot, not sure which one will in first
8) as we deprecate here and message thrown - I guess 8.7
9) same as 3 - just keeping scope
10) tests are split into existing classes, added \Drupal\KernelTests\Core\Entity\EntityLegacyTestTrait to not duplicate assertions also added \Drupal\Tests\comment\Kernel\CommentLegacyTest

Gábor Hojtsy’s picture

Status: Needs review » Needs work

@andypost: please don't use the new format until it is added to coding standards, and the deprecation policy is updated. Please stick to the current format.

andypost’s picture

Status: Needs work » Needs review
FileSize
19.96 KB
38.93 KB

Reverted messages

larowlan’s picture

Status: Needs review » Needs work

Patch at #18 is not consistent with the interdiff - still has the old format - possible that its the same patch as #16 - wrong file?

andypost’s picture

andypost’s picture

FileSize
38.89 KB

Another re-roll

Berdir’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/comment/src/Plugin/views/field/EntityLink.php
    @@ -61,7 +61,11 @@ public function preRender(&$values) {
         }
         if ($entities) {
    -      $this->build = entity_view_multiple($entities, $this->options['teaser'] ? 'teaser' : 'full');
    +      $entityTypeId = reset($entities)->getEntityTypeId();
    +      $viewMode = $this->options['teaser'] ? 'teaser' : 'full';
    +      $this->build = \Drupal::entityTypeManager()
    +        ->getViewBuilder($entityTypeId)
    +        ->viewMultiple($entities, $viewMode);
         }
    

    I'm quite sure this is not working at all as we have been lazy-rendering for a long time, so the array key in render() is never going to be set.

    Not really our problem here, but still..

  2. +++ b/core/modules/comment/tests/src/Kernel/CommentLegacyTest.php
    @@ -0,0 +1,104 @@
    +  /**
    +   * Tests deprecation of the comment_view() function.
    +   *
    +   * @expectedDeprecation comment_view() is deprecated in Drupal 8.0.0 and will be removed before Drupal 9.0.0. Use \Drupal::entityTypeManager()->getViewBuilder('comment')->view() instead. See https://www.drupal.org/node/3033656
    +   */
    +  public function testCommentView() {
    +    $entity = $this->createComment();
    +    $this->assertEntityView(comment_view($entity));
    +  }
    +
    

    in other cases we put multiple methods in the same, you can have multiple expected deprecation messages. don't need the separate method to create the comment then.

  3. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityLegacyTestTrait.php
    @@ -0,0 +1,31 @@
    +  /**
    +   * Asserts that build array of one entity is valid.
    +   *
    +   * @param array $build
    +   *   An array returned by entity builder for one entity.
    +   */
    +  protected function assertEntityView(array $build) {
    +    $this->assertNotEmpty($build);
    

    this seems pretty bogus, it's a one line assertion that we can also do easily in each case we test it.

  4. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityLegacyTestTrait.php
    @@ -0,0 +1,31 @@
    +   * @param array $build
    +   *   An array returned by entity builder for multiple entities.
    +   */
    +  protected function assertEntityViewMultiple(array $build) {
    +    unset($build['#sorted'], $build['#pre_render']);
    +    $this->assertEquals(2, count($build));
    

    also unsure about this one, why not just do assertCount(4, $build) and then the trait isn't needed.

andypost’s picture

Status: Needs work » Needs review
FileSize
10.88 KB
35.43 KB

removed trait and joined deprecations

#22.1 looks needs follow-up but not clear what to write in summary
#22.2 createComment() method could be inlined but IMO better to have it to minimize patch

Status: Needs review » Needs work

The last submitted patch, 23: 2974253-23.patch, failed testing. View results

andypost’s picture

queued again

andypost’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 23: 2974253-23.patch, failed testing. View results

andypost’s picture

Status: Needs work » Needs review

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Berdir’s picture

Status: Needs review » Needs work

Needs a reroll.

mikelutz’s picture

Title: Remove remaining usages of entity_view_multiple from core » Remove remaining usages of entity_view and entity_view_multiple from core
Issue summary: View changes
Status: Needs work » Needs review
FileSize
35.42 KB

Re-roll of #23 with no changes.

mikelutz’s picture

I also opened #3051491: Deprecate user_view and user_view_multiple because, why not! I assume that will need a re-roll after this gets in.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/modules/comment/comment.module
@@ -272,10 +272,15 @@ function comment_node_view_alter(array &$build, EntityInterface $node, EntityVie
  */
 function comment_view(CommentInterface $comment, $view_mode = 'full', $langcode = NULL) {
-  return entity_view($comment, $view_mode, $langcode);
+  @trigger_error("comment_view() is deprecated in Drupal 8.0.0 and will be removed before Drupal 9.0.0. Use \Drupal::entityTypeManager()->getViewBuilder('comment')->view() instead. See https://www.drupal.org/node/3033656", E_USER_DEPRECATED);
+  return \Drupal::entityTypeManager()
+    ->getViewBuilder('comment')
+    ->view($comment, $view_mode, $langcode);
 }
 

I'm still not sure if we should deprecate and update, in other places, we explicitly didn't touch deprecated code. The downside is that calling comment_view() would trigger two deprecation messages.

Not a big deal and it's done now already, so will leave the final decision on this to whoever is going to commit this (or not)

Beside that, I think this looks good now. All entity view functions are deprecated, have deprecation tests and all usages are updated.

larowlan’s picture

+++ b/core/modules/comment/comment.module
@@ -272,10 +272,15 @@ function comment_node_view_alter(array &$build, EntityInterface $node, EntityVie
 function comment_view(CommentInterface $comment, $view_mode = 'full', $langcode = NULL) {
-  return entity_view($comment, $view_mode, $langcode);
+  @trigger_error("comment_view() is deprecated in Drupal 8.0.0 and will be removed before Drupal 9.0.0. Use \Drupal::entityTypeManager()->getViewBuilder('comment')->view() instead. See https://www.drupal.org/node/3033656", E_USER_DEPRECATED);
+  return \Drupal::entityTypeManager()
+    ->getViewBuilder('comment')
+    ->view($comment, $view_mode, $langcode);

i'm ok with this, its dead code, its going to go away anyway

larowlan’s picture

Crediting @Gábor

larowlan’s picture

Status: Reviewed & tested by the community » Fixed

Committed f950d08 and pushed to 8.8.x. Thanks!

  • larowlan committed f950d08 on 8.8.x
    Issue #2974253 by andypost, legolasbo, mikelutz, Gábor Hojtsy: Remove...
jibran’s picture

Published the CR.

Status: Fixed » Closed (fixed)

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