Expand EntityDisplayBase with methods

There are no properties on this class, so I think we can close this issue.

There are no properties on that, but there are properties on the EntityDisplayBase class, those should have methods that are defined on EntityDisplayBaseInterface.

.

Working during mentored sprint : )

I believe I got all the public methods, I left the protected methods alone.
Maybe for a follow-up issue: the setOriginalMethod feels a bit weird being public, the name suggests it's set at creation, never to be changed. The only problem at this moment is that the createCopy method would break when the setOriginalMethod is changed to protected.

A bit too quick with the unassignment. Got some local feedback that EntityDisplay quickly got a new public property $status while I was working on this issue so I had to update it. Also added the forgotten @param doccomments for the setters.

Had a misunderstanding with Drupal's "magic" get/set methods.

Forgot to set the status =|

#9: expand-entitydisplay-with-methods-2030607-9.patch queued for re-testing.

Just saw you are using getTargetEntityType().
EntityDisplayModeBase has getTargetType() to return $this->targetEntityType.
Shouldn't the naming convention be consequent through all classes?

Just renamed the issue title to include EntityDisplayBase instead of EntityDisplay

I noticed that some of fago's comments to an issue I am working on also apply here. Check points 5. and 6 in the list of the linked comment.

Ok, so many things have changed, even one file used in previous patch is gone, so I start from the beginning:
I think the following properties need getters and setters:

public properties:
EntityDisplayBase::$targetEntityType
EntityDisplayBase::$mode
EntityDisplayBase::$originalMode
ConfigEntityBase::$langcode

protected properties: 
EntityDisplayBase::$content
EntityDisplayBase::$plugins
EntityDisplayBase::$displayContext
EntityDisplayBase::$pluginManager
EntityDisplayBase::$hidden

There is a getter, and I guess setter makes no sense for the following:
EntityDisplayBase::$fieldDefinitions

The Meta issue says: Protected properties can be listed in the sub-issues, and considered case by case if a getter makes sense. Somebody please help with these, I have no idea.

I think ConfigEntityBase::$langcode should not be touched here.

Uploading patch with getters and setters for the public properties.

Added back status and id properties, and fixed gettargetTypeId() to getTargetTypeId()

22: expand-entitydisplay-with-methods-2030607-22.patch queued for re-testing.

Re-roll. I didn't have time to add tests, but that does need to be done before this issue can be RTBC'ed.

Rerolled for PSR-4 and made properties protected to prove we're using the new methods everywhere we should. The interdiff is post PSR-4 reroll.

Fixed test fails and added tests for the new methods. Also revert change to remove $status property since the docs are valuable.

I have been reviweing this issue with @tstoeckler. A re-roll patch is updated.

+++ b/core/modules/field_ui/src/Tests/EntityDisplayTest.php
@@ -84,15 +84,15 @@ public function testEntityDisplayCRUD() {
+    EntityViewMode::create(array('id' => $display->getTargetEntityTypeId() . '.other_view_mode', 'targetEntityType' => $display->targetEntityType))->save();

$display->targetEntityType was missed here. This should be $display->getTargetEntityTypeId() like the first instance.

Furthermore in the EntityDisplayBaseTest.php the namespace for EntityDisplayBase, which is in \Drupal\Core\Entity should probably be used. I'm not sure if this should be done using a 'use' statement or if this is caused because the path in getMockForAbstractClass is given \Drupal\entity\EntityDisplayBase as path whereas EntityDisplayBase is at \Drupal\Core\Entity\EntityDisplayBase.

I can't run the updated test EntityDisplayBaseTest locally so I'm hoping they'll just pass on qa.d.o. EntityDisplayTest is now green. Changes are as per #35. In addition EntityDisplayBaseTest::getInfo has been removed as it is no longer used. (This suspicion was confirmed by Xano).

As always I forget the status : |

The patch makes sense. The tests make sense too. I think we need a change record now that we're in beta because we technically are breaking the API for public -> protected object variables.

Xano mentions this as RTBC.

1. +++ b/core/lib/Drupal/Core/Entity/Entity/EntityFormDisplay.php
   @@ -184,7 +184,7 @@ public function processForm($element, FormStateInterface $form_state, $form) {
   +    $extra_fields = \Drupal::entityManager()->getExtraFields($this->getTargetEntityTypeId(), $this->bundle);
   

   This is more of a comment about this code. Do we have the entity manager injected into this class? Might not be a big difference, but could be a follow-up issue.

2. +++ b/core/lib/Drupal/Core/Entity/EntityDisplayBase.php
   @@ -26,21 +26,21 @@
   +  protected  $id;
   

   We would probably need a change record for changing APIs at this point since we're in beta.

3. +++ b/core/lib/Drupal/Core/Entity/EntityDisplayBase.php
   @@ -338,7 +340,7 @@ public function getHighestWeight() {
   +    $weights = array_merge($weights, \Drupal::moduleHandler()->invokeAll('field_info_max_weight', array($this->getTargetEntityTypeId(), $this->bundle, $this->displayContext, $this->mode)));
   

   Could also use some injection love in a follow-up issue as well.

Xano mentions this as RTBC.

What did I do?

Can someone please update the issue summary with a section that describes the API changes? In this case you have to explain that the properties will no longer be public.

After that is done, the core maintainers can look at this and decide if it will get in or not.

Updated the issue summary. Leaving this to needs work as there was an @todo added to the patch, however the issue it points to has seemed to be resolved since. I'll assign this to myself and make a new patch to reflect this change.

Since this issue has been a long time coming, I would suggest to leave the change from public to protected of the variables (i.e. the actual API change) out of this for now and just add a @todo for now. Then we can tackle that in a smaller-scoped follow-up issue.

Re-rolled because of #2275659: Separate FieldableEntityInterface out of ContentEntityInterface.

Re-roll of #44. The only conflict I had to resolve was changing line 92 of EntityDisplayTest.php to look like this:

$this->assertEqual(array('config' => ...

+++ b/core/lib/Drupal/Core/Entity/Display/EntityDisplayInterface.php
@@ -90,4 +90,76 @@ public function getHighestWeight();
+  public function getTargetEntityTypeId();
...
+  public function getDisplayBundle();

Sorry, but I do not understand why we use getTargetEntityTypeId but getDisplayBundle. Shouldn't it be getTargetBundle?

I guess Kingdutch forgot to unassign, so unassigning...

The patch looks good to me. All the class variabls are protected and functions are added to set and get them. There are also PHPUnit tests added to test the functions. I have two nitpicks that I would like to be solved before a RTBC.

+++ b/core/lib/Drupal/Core/Entity/Display/EntityDisplayInterface.php
@@ -90,4 +90,76 @@ public function getHighestWeight();
+   * @return $this
+   */

In the doc blocks with the set functions the return value is set as this. Can we replace that? (see below)

+   * @return \Drupal\Core\Entity\Display\EntityDisplayInterface
+   *   The called EntityDisplay entity.

+++ b/core/lib/Drupal/Core/Entity/EntityDisplayBase.php
@@ -54,12 +54,14 @@
+   * @todo https://drupal.org/node/2286681 change to protected.
+   *
    * @var boolean

In the doc block is a reference to #2286681: Make public properties on ConfigEntityBase protected. This issue is fixed a long time ago. so can we remove this?

@return $this is valid docblock markup: https://www.drupal.org/coding-standards/docs#types It's different from returning any arbitrary EntityDisplayInterface.

@todo gone.

@Mile23: You are right. I have read it in the coding-standards. My mistake.

After green from test-server, I shall give it RTBC.

All the class variables are now protected.
The new getter and setter functions are in order and included in the correct interface.
Also the comments are in order.
There are PHPUnit test added.
The test-server gives it a green light.
So for me it is a RTBC.

1. +++ b/core/lib/Drupal/Core/Entity/EntityDisplayBase.php
   @@ -138,7 +138,7 @@ public function __construct(array $values, $entity_type) {
   -    return $this->targetEntityType . '.' . $this->bundle . '.' . $this->mode;
   +    return $this->getTargetEntityTypeId() . '.' . $this->bundle . '.' . $this->mode;
   

   This is inconsistent... if we're going to use a getter for one - why not all? It is okay for a class to know about it's own internals so i'd be fine with not doing this.

2. +++ b/core/lib/Drupal/Core/Entity/EntityDisplayBase.php
   @@ -171,7 +171,7 @@ public function calculateDependencies() {
   -      $field = FieldConfig::loadByName($this->targetEntityType, $this->bundle, $field_name);
   +      $field = FieldConfig::loadByName($this->getTargetEntityTypeId(), $this->bundle, $field_name);
   
   @@ -189,8 +189,8 @@ public function calculateDependencies() {
   -    if (\Drupal::entityManager()->hasHandler($this->targetEntityType, 'view_builder')) {
   -      \Drupal::entityManager()->getViewBuilder($this->targetEntityType)->resetCache();
   +    if (\Drupal::entityManager()->hasHandler($this->getTargetEntityTypeId(), 'view_builder')) {
   +      \Drupal::entityManager()->getViewBuilder($this->getTargetEntityTypeId())->resetCache();
   
   @@ -222,7 +222,7 @@ public function toArray() {
   -    $extra_fields = \Drupal::entityManager()->getExtraFields($this->targetEntityType, $this->bundle);
   +    $extra_fields = \Drupal::entityManager()->getExtraFields($this->getTargetEntityTypeId(), $this->bundle);
   
   @@ -327,7 +327,7 @@ public function getHighestWeight() {
   -    $weights = array_merge($weights, \Drupal::moduleHandler()->invokeAll('field_info_max_weight', array($this->targetEntityType, $this->bundle, $this->displayContext, $this->mode)));
   +    $weights = array_merge($weights, \Drupal::moduleHandler()->invokeAll('field_info_max_weight', array($this->getTargetEntityTypeId(), $this->bundle, $this->displayContext, $this->mode)));
   
   @@ -346,12 +346,12 @@ protected function getFieldDefinition($field_name) {
   -    if (!\Drupal::entityManager()->getDefinition($this->targetEntityType)->isSubclassOf('\Drupal\Core\Entity\FieldableEntityInterface')) {
   +    if (!\Drupal::entityManager()->getDefinition($this->getTargetEntityTypeId())->isSubclassOf('\Drupal\Core\Entity\FieldableEntityInterface')) {
   ...
   -      $definitions = \Drupal::entityManager()->getFieldDefinitions($this->targetEntityType, $this->bundle);
   +      $definitions = \Drupal::entityManager()->getFieldDefinitions($this->getTargetEntityTypeId(), $this->bundle);
   

   As above... all this change feels a bit unnecessary.

3. +++ b/core/lib/Drupal/Core/Entity/EntityDisplayBase.php
   @@ -399,4 +399,64 @@ public function onDependencyRemoval(array $dependencies) {
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function setTargetEntityTypeId($target_entity_type_id) {
   +    $this->set('targetEntityType', $target_entity_type_id);
   +    return $this;
   +  }
   ...
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function setMode($mode) {
   +    $this->set('mode', $mode);
   +    return $this;
   +  }
   

   These setters have no usages and I don' think it should be changeable.

4. +++ b/core/lib/Drupal/Core/Entity/EntityDisplayBase.php
   @@ -399,4 +399,64 @@ public function onDependencyRemoval(array $dependencies) {
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function setOriginalMode($original_mode) {
   +    $this->set('originalMode', $original_mode);
   +    return $this;
   +  }
   

   There are no usages of this... and setting this feels like an internal detail.

@alexpott: There was some question about set() in another similar issue. It has side-effects of using the plugin system. Whether or not we need those side effects is undocumented.

Patch does not apply.

Straight-up reroll of #49. Will address issues in #52 when the testbot comes back.

Straight-up reroll of #49. Will address issues in #52 when the testbot comes back.

I am applying changes as per #52. It is basically removing various method calls in EntityDisplayBase.php.

Fixing the error.

Removing the removed methods from the test case.

+++ b/core/lib/Drupal/Core/Entity/Display/EntityDisplayInterface.php
@@ -91,4 +91,45 @@ public function getHighestWeight();
+   * Return the original view or form mode that was requested (case of view/form modes
+   * being configured to fall back to the 'default' display).

Nitpick: The first line is too long. Can we move the word "modes" to the next line.

+++ b/core/tests/Drupal/Tests/Core/Config/Entity/EntityDisplayBaseTest.php
@@ -0,0 +1,58 @@
+ * @group Drupal

This line is not necessary.

+++ b/core/tests/Drupal/Tests/Core/Config/Entity/EntityDisplayBaseTest.php
@@ -0,0 +1,58 @@
+    $display = $this->getMockForAbstractClass('\Drupal\Core\Entity\EntityDisplayBase', array(), '', FALSE);
+    $display->set('targetEntityType', 'test');
+    $this->assertEquals('test', $display->getTargetEntityTypeId());

Can we rewrite this to:

    $mock = $this->getMockForAbstractClass('\Drupal\Core\Entity\EntityDisplayBase', array(), '', FALSE);

    $reflection = new \ReflectionProperty($mock, 'targetEntityType');
    $reflection->setAccessible(TRUE);
    $reflection->setValue($mock, 'test');

    $this->assertEquals('test', $mock->getTargetEntityTypeId());

The same for the other methods.

I think, we can't just move the "modes" to the next line, cuz all functions/methods need to start with a one-line description (https://www.drupal.org/node/1354#functions). So I've tried to split it correctly.

About #64:3 and testGetDisplayBundle(). Should we rewrite it too, or we need to replace $this->set('bundle', $bundle); in setDisplayBundle() ?

Looks good almost there.

+++ b/core/lib/Drupal/Core/Entity/Display/EntityDisplayInterface.php
@@ -107,8 +107,10 @@ public function getTargetEntityTypeId();
+   *
...
+   * display).

Nitpick: You can remove the first line. You can remove the closing bracket from the second line.

For the setDisplayBundle() you do the following:

$mock = $this->getMockForAbstractClass('\Drupal\Core\Entity\EntityDisplayBase', array(), '', FALSE);
$reflection = new \ReflectionProperty($mock, 'bundle');
$reflection->setAccessible(TRUE);
$mock->setDisplayBundle('test');
$this->assertEquals('test', $reflection->getValue($mock));

You can combine the tests for getDisplayBundle() and setDisplayBundle() into one test.

Changing as per #67 with a small change. For the comment on getOriginalMode(), I used the first line as I thought that makes more sense. The second line did not make much sense to me:

(case of view/form modes being configured to fall back to the 'default' display)

Looks good. Almost RTBC. One problem left.

+++ b/core/tests/Drupal/Tests/Core/Config/Entity/EntityDisplayBaseTest.php
@@ -60,4 +60,16 @@ public function testGetDisplayBundle() {
+    $reflection->setValue($mock, 'test');

You can remove this line. We are testing if the method setDisplayBundle() is working as it should. Now you overwrite the bundle property.

Changed, and a little bit more but I think/hope that shouldn't matter.

It all looks good to me.
There class variables are now protected and the five methods from the issue summery are added to the class.
The documentation is in order.
It gets a RTBC from me.

And back to RTBC.

Sorry, a couple additional nitpicks :

1. +++ b/core/lib/Drupal/Core/Entity/Display/EntityDisplayInterface.php
   @@ -91,4 +91,44 @@ public function getHighestWeight();
   +   * Returns the entity type for which this display mode is used.
   

   Slightly inaccurate - should be "for which this display is used".

2. +++ b/core/lib/Drupal/Core/Entity/Display/EntityDisplayInterface.php
   @@ -91,4 +91,44 @@ public function getHighestWeight();
   +   * @return string
   +   */
   +  public function getMode();
   ...
   +   * @return string
   +   */
   +  public function getOriginalMode();
   

   Missing description

3. +++ b/core/lib/Drupal/Core/Entity/Display/EntityDisplayInterface.php
   @@ -91,4 +91,44 @@ public function getHighestWeight();
   +  public function getDisplayBundle();
   ...
   +  public function setDisplayBundle($bundle);
   

   Not sure about that method name. The other methods don't have "Display".

   This goes hand in hand with getTargetEntityTypeId(), so maybe getTargetBundle() / setTargetBundle() to keep consistency ?

   Plus, FieldDefinitionInterface has getTargetEntityTypeId() / getTargetBundle() for something that its conceptually the same - the entity type and bundle this "thing" (display, field...) is about.

4. +++ b/core/lib/Drupal/Core/Entity/Entity/EntityFormDisplay.php
   @@ -185,7 +185,7 @@ public function processForm($element, FormStateInterface $form_state, $form) {
   -    $extra_fields = \Drupal::entityManager()->getExtraFields($this->targetEntityType, $this->bundle);
   +    $extra_fields = \Drupal::entityManager()->getExtraFields($this->getTargetEntityTypeId(), $this->bundle);
   

   No reason for this. We are in the class, we can still access a protected property - and we do throughout the rest of the class, including in that very line.

   So, for consistency, let's keep the property here.

Also, minor remark while we're in there: since they are basic getters and setters about the basic properties of the object, the new methods would be better off at the top of the interface, rather than added after the more advanced methods.
[edit: and same for the implementations in EntityDisplayBase]

Starting with a reroll. Will revert to Needs Work for review points after this patch passes.

For the record, I will also remove calls to set() method in specific setters as per #2030633-153: Expand FieldStorageConfig with methods point 5.

It seems I had to reroll before I could get to the changes. Same as #76.

Who would think it would need a reroll so soon. What I said in #76 still applies.

I am hoping this fixes most (or all) of the errors.

This will get you even farther :) I'm just updating all the new calls introduced in #2094499: Convert "Manage (form) display" forms to be the official entity edit forms for EntityView[Form]Display objects.

It all looks good to me.
There class variables are now protected and the five methods from the issue summery are added to the class.
The documentation is in order.
It gets a RTBC from me.

The remarks in #75 haven't been adressed yet.

Thanks for the review, @yched.

Hopefully deals with #74.1-4.

For the minor nitpick part: There isn't a coding standard about that, and we always tell everyone to localize your changes so the patch is easy to review.

The whole point of an interface is that you don't know or care how complex the implementation is, so there's no way to evaluate which is simple or complex. I've left it alone, because it's roughly alphabetical, with get and set together.

I've arranged the base class in the way I'd like to encounter it, which I hope is good.

Thanks @Mile23.

Method order : not a blocker, but I'd still prefer to move the methods up in the interface. They are introduced here, so it doesn't make the patch easier or harder to review, and having a similar order in the interface and in implementations is easier when navigating between the two.

Was about to RTBC, but looks the bot doesn't agree :-)

Catching the refactoring that NetBeans didn't.

Temptative RTBC if green

Ossum. :-)

I rerolled this patch for PSR-4 so I think it is fine for me to commit. Committed 2e92d42 and pushed to 8.0.x. Thanks!

The beta evaluation is in the meta issue.