This summary is adapted from #2818227: Undeprecate entity_get_(form_)display() or offer a real replacement for accuracy.

Problem/Motivation

#2474151: Mark procedural wrappers in entity.inc as deprecated deprecated entity_get_display() and entity_get_form_display(), but it did not offer real replacements, except to (eugh) copy the whole function body.

The point of those helper functions is that you don't know if an entity display exists, and they're designed to be created when used the first time. So it is very common to not know if one exists or not. The functionality is useful and should be preserved.

Proposed resolution

A) Keep the functions deprecated; there is no good reason for them to remain as procedural functions except as wrappers around a service.

B) Offer an actual replacement. According to the BC policy, it's OK to add new methods in minor releases.

Remaining tasks

Review and commit the patch.

User interface changes

None.

API changes

Two new methods added to EntityDisplayRepositoryInterface and all implementations -- currently EntityDisplayRepository and EntityManager. That means we need to add two new but already deprecated methods to EntityManager, but that is required because it implements that interface.

Data model changes

None.

CommentFileSizeAuthor
#83 interdiff-2367933-80-83.txt980 bytesphenaproxima
#83 2367933-83.patch247.57 KBphenaproxima
#81 interdiff-2367933-78-80.txt1.11 KBphenaproxima
#81 2367933-80.patch246.47 KBphenaproxima
#78 interdiff-2367933-73-78.txt16.12 KBphenaproxima
#78 2367933-78.patch245.78 KBphenaproxima
#73 interdiff_71-73.txt1.32 KBjohnwebdev
#73 2367933-73.patch234.98 KBjohnwebdev
#71 interdiff-2367933-69-71.txt1.32 KBvoleger
#71 2367933-71.patch235.14 KBvoleger
#69 interdiff-2367933-67-69.txt44.92 KBvoleger
#69 2367933-69.patch235.14 KBvoleger
#67 2367933-67.drupal.Move-entitygetformdisplay-to-the-entity-display-repository-interdiff.txt7.77 KBBerdir
#67 2367933-67.drupal.Move-entitygetformdisplay-to-the-entity-display-repository.patch272.34 KBBerdir
#66 interdiff.2367933.64-66.txt1.91 KBmikelutz
#66 2367933-66.drupal.Move-entitygetformdisplay-to-the-entity-display-repository.patch268.12 KBmikelutz
#64 interdiff.2367933.63-64.txt1.5 KBmikelutz
#64 2367933-64.drupal.Move-entitygetformdisplay-to-the-entity-display-repository.patch267.9 KBmikelutz
#63 interdiff.2367933.60-63.txt778 bytesmikelutz
#63 2367933-63.drupal.Move-entitygetformdisplay-to-the-entity-display-repository.patch267.92 KBmikelutz
#60 2367933-60.patch267.92 KBBerdir
#55 2367933-55.patch214.19 KBBerdir
#44 interdiff-2367933-43-44.txt7.86 KBphenaproxima
#44 2367933-44.patch218.98 KBphenaproxima
#43 2367933-43.patch209.73 KBphenaproxima
#40 interdiff-2367933-39-40.txt19 KBchr.fritsch
#40 2367933-40.patch215.11 KBchr.fritsch
#39 2367933-39.patch197.51 KBchr.fritsch
#37 2367933-37.patch196.13 KBjofitz
#32 2367933-32-do-not-test.patch16.78 KBphenaproxima
#32 interdiff-2367933-29-32.txt90.25 KBphenaproxima
#32 2367933-32.patch194.02 KBphenaproxima
#29 interdiff-2367933-27-29.txt5.5 KBphenaproxima
#29 2367933-29.patch194.81 KBphenaproxima
#27 interdiff-2367933-25-27.txt734 bytesphenaproxima
#27 2367933-27.patch190.65 KBphenaproxima
#25 2367933-25.patch190.65 KBphenaproxima
#23 interdiff-2367933-14-23.txt42.91 KBphenaproxima
#23 2367933-23.patch59.76 KBphenaproxima
#14 interdiff-2367933-12-14.txt1.03 KBphenaproxima
#14 2367933-14.patch14.22 KBphenaproxima
#12 interdiff-2367933-10-12.txt6.47 KBphenaproxima
#12 2367933-12.patch14.22 KBphenaproxima
#10 interdiff-2367933-8-10.txt2.14 KBphenaproxima
#10 2367933-10.patch11.13 KBphenaproxima
#8 2367933-8.patch10.84 KBphenaproxima
#3 interdiff.txt1013 bytesamateescu
#3 2367933-4.patch10.7 KBamateescu
#1 2367933.patch10.68 KBamateescu
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

amateescu’s picture

Status: Active » Needs review
FileSize
10.68 KB

Can't decide between getEntity(View|Form)Display() and get(View|Form)Display().. I chose the former for the initial patch.

Status: Needs review » Needs work

The last submitted patch, 1: 2367933.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
FileSize
10.7 KB
1013 bytes

#notmyday

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jhedstrom’s picture

Status: Needs review » Closed (outdated)

These have since been @deprecated for methods on the entity type manager.

Berdir’s picture

Status: Closed (outdated) » Needs work

They are, but that was IMHO a bad call, because there is no replacement for it, the point of those functions is that they create a display on demand if necessary and I don't want to duplicate that everywhere where we need it.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

phenaproxima’s picture

Title: Move entity_get_(form_)display() to the entity manager » Move entity_get_(form_)display() to the entity display repository
Version: 8.2.x-dev » 8.3.x-dev
Status: Needs work » Needs review
FileSize
10.84 KB

Arise, Lazarus. I agree with @Berdir -- these functions are hella useful. They deserve to be deprecated, but not removed -- they should be part of a service. That is what I have done; both are now methods of EntityDisplayRepositoryInterface.

Status: Needs review » Needs work

The last submitted patch, 8: 2367933-8.patch, failed testing.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
11.13 KB
2.14 KB

Bleh. Forgot to update EntityManager.

Berdir’s picture

Status: Needs review » Needs work

I think I'm getting old. Closed #2818227: Undeprecate entity_get_(form_)display() or offer a real replacement as a duplicate. that also says why adding those methods is OK in a minor release.

  1. +++ b/core/lib/Drupal/Core/Entity/EntityDisplayRepository.php
    @@ -243,4 +243,46 @@ public function clearDisplayModeInfo() {
    +  public function getViewDisplay($entity_type, $bundle, $view_mode = self::DEFAULT_DISPLAY) {
    ...
    +  public function getFormDisplay($entity_type, $bundle, $form_mode = self::DEFAULT_DISPLAY) {
    

    should this use static?

  2. +++ b/core/lib/Drupal/Core/Entity/EntityDisplayRepositoryInterface.php
    @@ -100,4 +107,84 @@ public function getFormModeOptionsByBundle($entity_type_id, $bundle);
    +   * Example usage:
    +   * - Set the 'body' field to be displayed and the 'field_image' field to be
    +   *   hidden on article nodes in the 'default' display.
    +   * @code
    +   * entity_get_display('node', 'article', 'default')
    +   *   ->setComponent('body', array(
    +   *     'type' => 'text_summary_or_trimmed',
    +   *     'settings' => array('trim_length' => '200')
    +   *     'weight' => 1,
    +   *   ))
    +   *   ->removeComponent('field_image')
    +   *   ->save();
    +   * @endcode
    

    the example needs to be updated to use the non-deprecated approach.

  3. +++ b/core/lib/Drupal/Core/Entity/EntityDisplayRepositoryInterface.php
    @@ -100,4 +107,84 @@ public function getFormModeOptionsByBundle($entity_type_id, $bundle);
    +   * @param string $view_mode
    +   *   (optional) The view mode.
    

    should mention what we default to.

  4. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
    @@ -512,4 +512,22 @@ public function getInstance(array $options) {
     
    +  /**
    +   * {@inheritdoc}
    +   *
    +   * @deprecated in Drupal 8.3.0, will be removed before Drupal 9.0.0.
    +   */
    +  public function getViewDisplay($entity_type, $bundle, $view_mode = self::DEFAULT_DISPLAY) {
    +    return $this->container->get('entity_display.repository')->getViewDisplay($entity_type, $bundle, $view_mode);
    +  }
    +
    

    We don't need add deprecated methods for something that was never here in the first place :)

Should we do a few example conversions? Looks like we only have a handful of use cases outside of functions/tests, so possibly we could convert those? (some field_ui forms)

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
14.22 KB
6.47 KB

All fixed except for:

#1. We cannot use static to refer to interface constants because static is a compile-time construct.
#4. EntityManager must implement the methods, because it implements EntityDisplayRepositoryInterface.

Status: Needs review » Needs work

The last submitted patch, 12: 2367933-12.patch, failed testing.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
14.22 KB
1.03 KB

D'oh. Let's try that again.

Berdir’s picture

Status: Needs review » Needs work

I think this is ready, but this possibly needs a change record to clarify that those functions are still deprecated but now have an actual replacement.

We probably also want to take over parts or all of the issue summary of #2818227: Undeprecate entity_get_(form_)display() or offer a real replacement, including the reasoning why adding those methods to an existing interface is OK, per our BC rules.

phenaproxima’s picture

Status: Needs work » Needs review
Berdir’s picture

Status: Needs review » Needs work

Copied from IRC, in case you don't see it there:

I think you misunderstood "We probably also want to take over parts or all of the issue summary of ..." :)

I'm saying this has a very old and wrong issue summary. we should update it and the one from that other issue should be a good starting point. Or just that, now that you updated it there ;)

phenaproxima’s picture

Issue summary: View changes
phenaproxima’s picture

Status: Needs work » Needs review
phenaproxima’s picture

Issue summary: View changes
Berdir’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Thanks.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

I think we should use this issue to add the missing explicit test coverage of the new methods on the Entity Display repository.

+++ b/core/lib/Drupal/Core/Entity/EntityManager.php
@@ -512,4 +512,22 @@ public function getInstance(array $options) {
+  /**
+   * {@inheritdoc}
+   *
+   * @deprecated in Drupal 8.3.0, will be removed before Drupal 9.0.0.
+   */
+  public function getViewDisplay($entity_type, $bundle, $view_mode = self::DEFAULT_VIEW_MODE) {
+    return $this->container->get('entity_display.repository')->getViewDisplay($entity_type, $bundle, $view_mode);
+  }
+
+  /**
+   * {@inheritdoc}
+   *
+   * @deprecated in Drupal 8.3.0, will be removed before Drupal 9.0.0.
+   */
+  public function getFormDisplay($entity_type, $bundle, $form_mode = self::DEFAULT_FORM_MODE) {
+    return $this->container->get('entity_display.repository')->getFormDisplay($entity_type, $bundle, $form_mode);
+  }

Ugh what a shame that this has to be done. But no avoiding that.

Can we search for and if it does not exist create followups to remove the usages of entity_get_display() and entity_get_form_display() - there are plenty.

Just a random thought but over in #2834268: DX of \Drupal::config() is extremely confusing: doesn't complain when requesting non-existing config people are having issues with the auto-create if it does not exist approach. Will we have an issue with that here?

phenaproxima’s picture

To paraphrase Dr. Zoidberg, what's the point of keeping the usages around after this patch? It's scope creep, but IMHO we should just remove all usages now. Like taking off a band-aid.

Here's a WIP patch that removes many usages from core, mostly in tests.

phenaproxima’s picture

Issue tags: +Kill includes, +API clean-up
phenaproxima’s picture

Status: Needs work » Needs review
FileSize
190.65 KB

Now that was a dirty job. Someone should call Mike Rowe.

Anyhoo, this patch removes all usages of entity_get_display() and entity_get_form_display() -- most of which were in tests. Let's see how many failures we get!

Decided not to post an interdiff because there's nothing really special going on here; it's just changing function calls to service calls. If all goes well, will add explicit tests of both methods.

Status: Needs review » Needs work

The last submitted patch, 25: 2367933-25.patch, failed testing.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
190.65 KB
734 bytes

I forgot a semicolon.

Status: Needs review » Needs work

The last submitted patch, 27: 2367933-27.patch, failed testing.

phenaproxima’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
194.81 KB
5.5 KB

Fixed borked tests and added explicit test coverage of both new methods. Should be good to go!

Berdir’s picture

Hm. I'm not sure if converting all tests in here is a good idea, makes for a prety huge patch.

See also #2066993: Use \Drupal consistently in tests for discussion on $this->container vs \Drupal in tests.

phenaproxima’s picture

It was either going to be a huge patch in a follow-up issue, or a huge patch in this one. I figure we should just get it done now and be through with it.

phenaproxima’s picture

After discussion with @Berdir on IRC, here are two things that should help this get going:

1) A review-only patch containing the relevant changes, without any of the updated code.
2) Calls to $this->container were replaced with \Drupal.

slashrsm’s picture

A nit:

+++ b/core/includes/entity.inc
@@ -450,48 +450,11 @@ function entity_view_multiple(array $entities, $view_mode, $langcode = NULL, $re
  * @deprecated as of Drupal 8.0.x, will be removed before Drupal 9.0.0.
...
+ * Use EntityDisplayRepositoryInterface::getViewDisplay() instead.

@@ -529,47 +492,13 @@ function entity_get_display($entity_type, $bundle, $view_mode) {
+ * @deprecated as of Drupal 8.0.x, will be removed before Drupal 9.0.0. Use
+ * EntityDisplayRepositoryInterface::getFormDisplay() instead.

I believe that the second line needs to be indented.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mpdonadio’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Needs a re-roll, likely b/c #2776975: March 3, 2017: Convert core to array syntax coding standards for Drupal 8.3.x RC phase, but looks like some other conflicts, too.

jofitz’s picture

Assigned: Unassigned » jofitz

I have a feeling I might regret this, but I'll give it a shot...

jofitz’s picture

Assigned: jofitz » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
196.13 KB

I was right - I did regret doing that! But here is the fruit of my labour.

Version: 8.4.x-dev » 8.5.x-dev

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

chr.fritsch’s picture

Just a re-roll on 8.5.x

chr.fritsch’s picture

I removed all the remaining occurrences of entity_get_display and entity_get_form_display

Status: Needs review » Needs work

The last submitted patch, 40: 2367933-40.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

phenaproxima’s picture

Issue tags: +Needs reroll

Okay...

phenaproxima’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
209.73 KB

Re-rolled!

phenaproxima’s picture

Removed more calls to entity_get_display() and entity_get_form_display().

The last submitted patch, 43: 2367933-43.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Status: Needs review » Needs work

The last submitted patch, 44: 2367933-44.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

andypost’s picture

This needs rework
1) no reason to add methods to deprecated EM
2) displays just needs custom storage classes to hide logic with entity creation of default display

  1. +++ b/core/includes/entity.inc
    @@ -450,48 +450,11 @@ function entity_view_multiple(array $entities, $view_mode, $langcode = NULL, $re
     function entity_get_display($entity_type, $bundle, $view_mode) {
    -  // Try loading the display from configuration.
    -  $display = EntityViewDisplay::load($entity_type . '.' . $bundle . '.' . $view_mode);
    -
    -  // If not found, create a fresh display object. We do not preemptively create
    -  // new entity_view_display configuration entries for each existing entity type
    -  // and bundle whenever a new view mode becomes available. Instead,
    -  // configuration entries are only created when a display object is explicitly
    -  // configured and saved.
    -  if (!$display) {
    -    $display = EntityViewDisplay::create([
    -      'targetEntityType' => $entity_type,
    -      'bundle' => $bundle,
    -      'mode' => $view_mode,
    -      'status' => TRUE,
    -    ]);
    -  }
    -
    -  return $display;
    +  return \Drupal::service('entity_display.repository')
    +    ->getViewDisplay($entity_type, $bundle, $view_mode);
    

    Maybe better to override load() method on the entities or use post_load hook?

  2. +++ b/core/lib/Drupal/Core/Entity/EntityDisplayRepositoryInterface.php
    @@ -100,4 +114,86 @@ public function getFormModeOptionsByBundle($entity_type_id, $bundle);
    +  public function getViewDisplay($entity_type, $bundle, $view_mode = self::DEFAULT_VIEW_MODE);
    ...
    +  public function getFormDisplay($entity_type, $bundle, $form_mode = self::DEFAULT_FORM_MODE);
    
    +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
    @@ -714,4 +714,22 @@ public function getInstance(array $options) {
    +   * @deprecated in Drupal 8.3.0, will be removed before Drupal 9.0.0.
    ...
    +  public function getViewDisplay($entity_type, $bundle, $view_mode = self::DEFAULT_VIEW_MODE) {
    +    return \Drupal::service('entity_display.repository')->getViewDisplay($entity_type, $bundle, $view_mode);
    ...
    +   * @deprecated in Drupal 8.3.0, will be removed before Drupal 9.0.0.
    ...
    +  public function getFormDisplay($entity_type, $bundle, $form_mode = self::DEFAULT_FORM_MODE) {
    +    return \Drupal::service('entity_display.repository')->getFormDisplay($entity_type, $bundle, $form_mode);
    

    this methods are not defined initially, no reason to add deprecated methods to deprecated interface

phenaproxima’s picture

This needs rework
1) no reason to add methods to deprecated EM
2) displays just needs custom storage classes to hide logic with entity creation of default display

Either way is valid. We need a framework manager to break this stalemate.

So the question I pose to them is this: should we...

  • Move entity_get_display() and entity_get_form_display() to new methods on EntityDisplayRepositoryInterface (getViewDisplay() and getFormDisplay(), respectively), which would involve implementing them on EntityManager as well, since it implements that interface?
  • Move the logic into the storage handlers for view and form displays? Maybe something like $display_storage->load($id, $create_if_not_exists = FALSE)?

Tagging for framework manager review before we proceed with this.

andypost’s picture

Move the logic into the storage handlers for view and form displays? Maybe something like $display_storage->load($id, $create_if_not_exists = FALSE)

I prefer this approach but not sure optional argument allowed by interface

phenaproxima’s picture

@andypost: My only hesitation with that approach is that it's not clear how we should handle loadMultiple(). Should the create-if-not-exists flag work only for load(), or for loadMultiple() as well? Because it might be pretty tricky to make it work for loadMultiple() too.

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

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

andypost’s picture

Faced with that again working on domain_entity (menu) module - some distributions are missing form displays for menu link entities...

Btw looking through usage of "EGFD" it is used exactly for such ugly cases when entities have no default display
Probably for 9.x we need to prevent having "missing but default" display usage at all

As workaround entity repository looks a best place cos it already has ETM injected & implements similar "hacky methods" which works abroad all entities

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.

voleger’s picture

Berdir’s picture

Status: Needs work » Needs review
Issue tags: -Needs framework manager review
FileSize
214.19 KB

> Tagging for framework manager review before we proceed with this.

I believe that this is not a framework-level decision but one for an entity subsystem manager. I happen to be one and I prefer to have them on the EntityDisplayRepository.

Here is a rebase, conflicted on a bunch of tests but most things still applied thanks to applying the patch to a commit from 2017 and then letting git rebase do its magic. A few conflicts I just dropped, they'll need to be redone in their new locations.

Originally, I was against doing this in a single 200kb patch, but with our new rules, this is now the way it has to be.

Next steps:
* Add @trigger_error() statements to the deprecated functions
* Make changed constructor arguments optional, again with @trigger_error()
* Update all remaining instances.

Status: Needs review » Needs work

The last submitted patch, 55: 2367933-55.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

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.

andypost’s picture

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

The most usage of new API in install & tests (which mix "full" and "default") - also it makes in interface that default display always exists (imo no reason to separate default view mode variable name and have 2 constants in interface)
Also I find it useful, for search module at east, to get display from repository/factory in controllable status (true or false)

+++ b/core/lib/Drupal/Core/Entity/EntityDisplayRepository.php
@@ -243,4 +243,46 @@ public function clearDisplayModeInfo() {
+        'status' => TRUE,
...
+        'status' => TRUE,

+++ b/core/lib/Drupal/Core/Entity/EntityDisplayRepositoryInterface.php
@@ -7,6 +7,20 @@
 interface EntityDisplayRepositoryInterface {
...
+   * The default view mode ID.
...
+  const DEFAULT_VIEW_MODE = 'default';
...
+   * The default form mode ID.
...
+  const DEFAULT_FORM_MODE = 'default';

@@ -100,4 +114,86 @@ public function getFormModeOptionsByBundle($entity_type_id, $bundle);
+   * settings for the bundle; if not, the 'default' display is used instead.
...
+   * @param string $view_mode
+   *   (optional) The view mode. Defaults to self::DEFAULT_VIEW_MODE.
...
+  public function getViewDisplay($entity_type, $bundle, $view_mode = self::DEFAULT_VIEW_MODE);
...
+   * @param string $form_mode
+   *   (optional) The form mode. Defaults to self::DEFAULT_FORM_MODE.
...
+  public function getFormDisplay($entity_type, $bundle, $form_mode = self::DEFAULT_FORM_MODE);

Maybe better to factor interface to ($entity_type_id, $bundle, $mode='default', $status=TRUE) - view or form already used in method name

larowlan’s picture

I believe that this is not a framework-level decision but one for an entity subsystem manager. I happen to be one and I prefer to have them on the EntityDisplayRepository.

I think they should be on EntityDisplayRepository too, and if that means we need to create a new interface to avoid adding them to EntityManager, then let's do that

Berdir’s picture

Status: Needs work » Needs review
FileSize
267.92 KB

Re #58: Yeah, agree, the constants can be merged. But I don't think the status is necessary.

Re #59: We actually added quite a new methods to EntityManager, do we really need to make an exception here? I find that new interfaces are tricky as all implementations need to implement that anyway or the service woudln't work. Recent examples: #2984782: Add an API for converting entity type schemas to (non-)revisionable/(non-)translatable, with or without data and #2942907: Entity system does not provide an API for retrieving an entity variant that is safe for editing.

Just moving this a bit along, added @trigger_error() and updated the remaining usages. Messed up the interdiff by working on it during rebase, but it's just more of the same.

alexpott’s picture

Priority: Normal » Major

I'm upgrading this to a major task since this will help people trying to fix deprecated code since as we know atm the procedural methods have no direct counterparts and that makes getting your code ready for Drupal 9 harder than it should be.

mikelutz’s picture

mikelutz’s picture

Typo fix to move this forward. Looking to fix the deprecation in contrib, and would prefer to fix it with the final api if possible.

mikelutz’s picture

The last submitted patch, 63: 2367933-63.drupal.Move-entitygetformdisplay-to-the-entity-display-repository.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

mikelutz’s picture

Berdir’s picture

Added legacy tests, also reformatted things a bit, I didn't really like that "single line" implementation in EntityViewDisplay.

I think this is ready for final reviews.

amateescu’s picture

Version: 8.7.x-dev » 8.8.x-dev
Status: Needs review » Needs work

This looks really good to me. I mostly found nitpicks but setting to NW for point #5 :)

  1. +++ b/core/includes/entity.inc
    @@ -440,49 +438,13 @@ function entity_view_multiple(array $entities, $view_mode, $langcode = NULL, $re
    + * @deprecated as of drupal:8.0.0, will be removed before drupal:9.0.0. Use
    + *   EntityDisplayRepositoryInterface::getViewDisplay() instead.
    
    @@ -520,47 +482,14 @@ function entity_get_display($entity_type, $bundle, $view_mode) {
    + * @deprecated as of drupal:8.0.0, will be removed before drupal:9.0.0. Use
    + * EntityDisplayRepositoryInterface::getFormDisplay() instead.
    

    as of drupal:8.0.0 -> in drupal:8.0.0?

  2. +++ b/core/lib/Drupal/Core/Entity/EntityDisplayRepository.php
    @@ -243,4 +243,50 @@ public function clearDisplayModeInfo() {
    +      $entity_view_display =$storage->create([
    ...
    +      $entity_form_display =$storage->create([
    

    Missing space before $storage :)

  3. +++ b/core/modules/datetime/src/Tests/DateTestBase.php
    @@ -28,7 +28,7 @@
    +   * An array of display options to pass to EntityDisplayRepositoryInterface::getViewDisplay()
    
    +++ b/core/modules/datetime/tests/src/Functional/DateTestBase.php
    @@ -24,7 +24,7 @@
    +   * An array of display options to pass to EntityDisplayRepositoryInterface::getViewDisplay()
    

    Exceeds 80 chars.

  4. +++ b/core/modules/field_ui/tests/src/Kernel/EntityDisplayTest.php
    @@ -134,20 +134,22 @@ public function testEntityDisplayCRUDSort() {
    +   * Tests EntityDisplayRepositoryInterface::getViewDisplay().
    
    +++ b/core/modules/field_ui/tests/src/Kernel/EntityFormDisplayTest.php
    @@ -28,20 +28,24 @@ protected function setUp() {
    +   * Tests EntityDisplayRepositoryInterface::getFormDisplay().
    

    Since we're changing these lines anyway, we can use @covers ....

  5. +++ b/core/modules/quickedit/src/Tests/QuickEditAutocompleteTermTest.php
    @@ -0,0 +1,218 @@
    +class QuickEditAutocompleteTermTest extends WebTestBase {
    
    +++ b/core/modules/quickedit/src/Tests/QuickEditLoadingTest.php
    @@ -0,0 +1,594 @@
    +class QuickEditLoadingTest extends WebTestBase {
    

    I'm guessing these tests are added because of a bad merge, so they should be removed from the patch.

voleger’s picture

Status: Needs review » Needs work

The last submitted patch, 69: 2367933-69.patch, failed testing. View results

voleger’s picture

Status: Needs work » Needs review
FileSize
235.14 KB
1.32 KB

Status: Needs review » Needs work

The last submitted patch, 71: 2367933-71.patch, failed testing. View results

johnwebdev’s picture

Status: Needs work » Needs review
FileSize
234.98 KB
1.32 KB

Fixes tests :)

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

I think we're good to go now!

larowlan’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/includes/entity.inc
    @@ -440,49 +438,15 @@ function entity_view_multiple(array $entities, $view_mode, $langcode = NULL, $re
    + * @deprecated in drupal:8.0.0 and will be removed from drupal:9.0.0. Use
    ...
    +  @trigger_error('entity_get_display() is deprecated in drupal:8.8.0. It will be removed before drupal:9.0.0. Use \Drupal::service(\'entity_display.repository\')->getViewDisplay() instead. See https://www.drupal.org/node/2835616', E_USER_DEPRECATED);
    
    @@ -520,47 +484,15 @@ function entity_get_display($entity_type, $bundle, $view_mode) {
    + * @deprecated in drupal:8.0.0 and will be removed from drupal:9.0.0. Use
    ...
    +  @trigger_error('entity_get_form_display() is deprecated in drupal:8.8.0. It will be removed before drupal:9.0.0. Use \Drupal::service(\'entity_display.repository\')->getFormDisplay() instead. See https://www.drupal.org/node/2835616', E_USER_DEPRECATED);
    

    We have a mismatch in versions here.

    This was actually deprecated in #2474151: Mark procedural wrappers in entity.inc as deprecated which was 8.0.0, so we should probably retain that?

  2. +++ b/core/lib/Drupal/Core/Entity/EntityDisplayRepositoryInterface.php
    @@ -7,6 +7,13 @@
    +   *
    +   * @var string
    

    do we normally document the type of constants using @var? Looked at a few samples in core, e.g. CommentItemInterface and FieldStorageDefinitionInterface and didn't see it.

  3. +++ b/core/modules/comment/src/CommentManager.php
    @@ -129,15 +129,18 @@ public function addBodyField($comment_type_id) {
    +      $display_repository = \Drupal::service('entity_display.repository');
    

    we can inject this

  4. +++ b/core/modules/field/tests/src/Kernel/Uri/UriItemTest.php
    --- a/core/modules/field_ui/src/Form/EntityFormDisplayEditForm.php
    +++ b/core/modules/field_ui/src/Form/EntityFormDisplayEditForm.php
    
    +++ b/core/modules/field_ui/src/Form/EntityFormDisplayEditForm.php
    @@ -55,7 +55,8 @@ protected function buildFieldRow(FieldDefinitionInterface $field_definition, arr
    -    return entity_get_form_display($entity_type_id, $bundle, $mode);
    +    return \Drupal::service('entity_display.repository')
    +      ->getFormDisplay($entity_type_id, $bundle, $mode);
    
    --- a/core/modules/field_ui/src/Form/EntityViewDisplayEditForm.php
    +++ b/core/modules/field_ui/src/Form/EntityViewDisplayEditForm.php
    
    +++ b/core/modules/field_ui/src/Form/EntityViewDisplayEditForm.php
    @@ -88,7 +88,7 @@ protected function buildExtraFieldRow($field_id, $extra_field) {
    -    return entity_get_display($entity_type_id, $bundle, $mode);
    +    return $this->entityDisplayRepository->getViewDisplay($entity_type_id, $bundle, $mode);
    
    +++ b/core/modules/field_ui/src/Form/FieldStorageAddForm.php
    @@ -461,7 +461,7 @@ protected function configureEntityFormDisplay($field_name, $widget_id = NULL, ar
    -    entity_get_form_display($this->entityTypeId, $this->bundle, 'default')
    +    \Drupal::service('entity_display.repository')->getFormDisplay($this->entityTypeId, $this->bundle, 'default')
    
    @@ -487,7 +487,7 @@ protected function configureEntityViewDisplay($field_name, $formatter_id = NULL,
    -    entity_get_display($this->entityTypeId, $this->bundle, 'default')
    +    \Drupal::service('entity_display.repository')->getViewDisplay($this->entityTypeId, $this->bundle)
    

    And here too

  5. +++ b/core/modules/media/tests/src/Traits/MediaTypeCreationTrait.php
    --- a/core/modules/migrate/src/Plugin/migrate/destination/PerComponentEntityDisplay.php
    +++ b/core/modules/migrate/src/Plugin/migrate/destination/PerComponentEntityDisplay.php
    
    +++ b/core/modules/migrate/src/Plugin/migrate/destination/PerComponentEntityDisplay.php
    +++ b/core/modules/migrate/src/Plugin/migrate/destination/PerComponentEntityDisplay.php
    @@ -56,7 +56,8 @@ class PerComponentEntityDisplay extends ComponentEntityDisplayBase {
    
    @@ -56,7 +56,8 @@ class PerComponentEntityDisplay extends ComponentEntityDisplayBase {
        * {@inheritdoc}
        */
       protected function getEntity($entity_type, $bundle, $view_mode) {
    -    return entity_get_display($entity_type, $bundle, $view_mode);
    +    return \Drupal::service('entity_display.repository')
    +      ->getViewDisplay($entity_type, $bundle, $view_mode);
    
    +++ b/core/modules/migrate/src/Plugin/migrate/destination/PerComponentEntityFormDisplay.php
    +++ b/core/modules/migrate/src/Plugin/migrate/destination/PerComponentEntityFormDisplay.php
    @@ -49,7 +49,8 @@ class PerComponentEntityFormDisplay extends ComponentEntityDisplayBase {
    
    @@ -49,7 +49,8 @@ class PerComponentEntityFormDisplay extends ComponentEntityDisplayBase {
        * {@inheritdoc}
        */
       protected function getEntity($entity_type, $bundle, $form_mode) {
    -    return entity_get_form_display($entity_type, $bundle, $form_mode);
    +    return \Drupal::service('entity_display.repository')
    +      ->getFormDisplay($entity_type, $bundle, $form_mode);
    
    +++ b/core/modules/node/node.tokens.inc
    --- a/core/modules/node/src/Plugin/views/wizard/Node.php
    +++ b/core/modules/node/src/Plugin/views/wizard/Node.php
    
    +++ b/core/modules/node/src/Plugin/views/wizard/Node.php
    +++ b/core/modules/node/src/Plugin/views/wizard/Node.php
    @@ -213,7 +213,8 @@ protected function buildFilters(&$form, FormStateInterface $form_state) {
    
    @@ -213,7 +213,8 @@ protected function buildFilters(&$form, FormStateInterface $form_state) {
         }
         $tag_fields = [];
         foreach ($bundles as $bundle) {
    -      $display = entity_get_form_display($this->entityTypeId, $bundle, 'default');
    +      $display = \Drupal::service('entity_display.repository')
    +        ->getFormDisplay($this->entityTypeId, $bundle);
    

    and here

Berdir’s picture

1. Missed the inconsistent one. IMHO, the version defines when it is safe to use the documented replacement. That is 8.8.0, the earlier deprecation was was a a lie as it didn't have an actual replacement.

Will fix that and the injections when I find some time, but someone else can try that too if they want.

alexpott’s picture

I agree with the rationale for changing the deprecation to 8.8.0 as that’s when you can use the replacement. The other question is should we file an issue to undeprecate these methods in 8.7.x as well - which I would be in favour of as a followup.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
245.78 KB
16.12 KB

Addressing #75:

  1. Discussed in Slack with @Berdir, @alexpott, and @amateescu. We agreed that the least confusing thing to do here is to say we deprecated the function in 8.8.0, since that's when an actual replacement was added.
  2. Both styles are found in core (LayoutBuilderEvents leaps to mind as an example of a class with type-annotated constants). I think that older code tends to omit the type annotations, and newer code has them. So I left this as-is. Perhaps a follow-up to formally define this in the coding standards would be useful? :)
  3. Fixed all injection points you mentioned. Hopefully this patch will still pass tests. One spot I didn't fix was MediaTypeCreationTrait, because that is a testing trait and tests, as far as I know, don't really "do" dependency injection as such.
Berdir’s picture

  1. +++ b/core/includes/entity.inc
    @@ -484,7 +484,7 @@
      *
    - * @deprecated in drupal:8.0.0 and will be removed from drupal:9.0.0. Use
    + * @deprecated in drupal:8.8.0 and will be removed from drupal:9.0.0. Use
      * EntityDisplayRepositoryInterface::getFormDisplay() instead.
      *
    

    indentation wrong on the second line.

  2. +++ b/core/modules/comment/src/CommentManager.php
    @@ -100,6 +110,12 @@
         }
         $this->entityFieldManager = $entity_field_manager;
    +
    +    if (!$entity_display_repository) {
    +      @trigger_error('The entity_display.repository service must be passed to CommentManager::__construct(), it is required before Drupal 9.0.0. See https://www.drupal.org/node/2835616.', E_USER_DEPRECATED);
    +      $entity_display_repository = \Drupal::service('entity_display.repository');
    +    }
    +    $this->entityDisplayRepository = $entity_display_repository;
       }
    

    It looks like comment.services.yml hasn't been updated for the new argument?

Status: Needs review » Needs work

The last submitted patch, 78: 2367933-78.patch, failed testing. View results

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
246.47 KB
1.11 KB

Ah, well spotted! Both fixed. I bet this will also cause way fewer tests to fail. :)

Status: Needs review » Needs work

The last submitted patch, 81: 2367933-80.patch, failed testing. View results

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
247.57 KB
980 bytes

Fixing the final broken test.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

I think the review has been addressed. Not sure about the constant myself, I've never added @var to them before, but it indeed looks there is no standard and if there are recent examples in core that have a type then I'll defer that decision to whoever is going to commit this. Can easily be removed before commit too.

The only examples I found is https://www.drupal.org/docs/develop/standards/api-documentation-and-comm... in the context of an event and https://www.drupal.org/docs/develop/standards/api-documentation-samples#..., but saw many empty and out of date examples on that page, including "Member constant" a bit up.

larowlan’s picture

Issue credits

larowlan’s picture

Status: Reviewed & tested by the community » Fixed

fixed on commit

index 774ecd35aa..dc7c475783 100644
--- a/core/tests/Drupal/Tests/Core/Entity/EntityManagerTest.php
+++ b/core/tests/Drupal/Tests/Core/Entity/EntityManagerTest.php
@@ -489,8 +489,8 @@ public function testGetViewDisplay() {
    * @expectedDeprecation EntityManager::getFormDisplay() is deprecated in drupal:8.8.0 and will be removed before Drupal 9.0.0. Use \Drupal::service('entity_display.repository')->getFormDisplay() instead.
    */
   public function testGetFormDisplay() {
-    $Form_display = $this->prophesize(EntityFormDisplayInterface::class)->reveal();
-    $this->entityDisplayRepository->getFormDisplay('entity_test', 'bundle', 'default')->shouldBeCalled()->willReturn($Form_display);
+    $form_display = $this->prophesize(EntityFormDisplayInterface::class)->reveal();
+    $this->entityDisplayRepository->getFormDisplay('entity_test', 'bundle', 'default')->shouldBeCalled()->willReturn($form_display);
     $this->assertInstanceOf(EntityFormDisplayInterface::class, $this->entityManager->getFormDisplay('entity_test', 'bundle', 'default'));
   }

Committed da94d7f and pushed to 8.8.x. Thanks!
Published change record

  • larowlan committed da94d7f on 8.8.x
    Issue #2367933 by phenaproxima, mikelutz, Berdir, voleger, amateescu,...

Status: Fixed » Closed (fixed)

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