Problem/Motivation

Part of meta-issue #2016679: Expand Entity Type interfaces to provide methods, protect the properties.

See the detailed explanations there and look at the issues that already have patches or were commited.

Add get*, set* and additional methods as it makes sense to replace the public properties (e.g. isSomething() and something())

Remaining tasks

Contributor tasks needed
Task Novice task? Contributor instructions Complete?
Reroll the patch if it no longer applies. Instructions DONE
Add automated tests Small tests to make sure setters work properly. Instructions DONE
Draft a change record for the API changes Instructions
Review patch to ensure that it fixes the issue, stays within scope, is properly documented, and follows coding standards Instructions

User interface changes

NONE

API changes

EntityDisplayModeInterface now uses setTargetEntityTypeId() and getTargetEntityTypeId() to get and set its $targetEntityType property.
The EntityDisplayModeBase class implements the EntityDisplayModeInterface class, and these changes are reflected there too.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

plopesc’s picture

Title: Expand EntityFormDisplay with methods » Expand EntityViewMode with methods

Part of meta-issue #2016679: Expand Entity Type interfaces to provide methods, protect the properties.

See the detailed explanations there and look at the issues that already have patches or were commited.

Add get*, set* and additional methods as it makes sense to replace the public properties (e.g. isSomething() and something())

hernani’s picture

Assigned: Unassigned » hernani

Working in Prague Sprint

hernani’s picture

Assigned: hernani » Unassigned
balagan’s picture

Assigned: Unassigned » balagan
Issue summary: View changes
balagan’s picture

I am not sure if there is anything to do. The properties can be found here.
I have found the following getter and setter methods up in the hierarchy tree:

EntityDisplayModeBase class
getters:
->getTargetType()

ConfigEntityBase class
getters:
->getOriginalId()
->isSyncing()
->status()
setters:
->setOriginalId()
->setSyncing()
->setStatus()

Entity class
getters:
->entityType()
->routeProvider()
->label()
setters:
->enforceIsNew()

Maybe a setLabel(), setTargetType(), setRouteProvider(), setEntityType() method is meaningful, although I am not sure about especially the two latter.

balagan’s picture

Status: Active » Needs review
FileSize
2.21 KB

Enclosed a patch with setLabel() and setTargetType(), although I am not sure about how to name them. See this comment. I have also changed the properties to protected.

balagan’s picture

Enclosed a patch with the same changes as in the previous patch, but properties remain public.

balagan’s picture

Status: Needs review » Needs work
balagan’s picture

Status: Needs work » Needs review
FileSize
2.23 KB

In previous patch I forgot to return a value, fixed that. Enclosing patch with protected properties.

balagan’s picture

Same patch (setLabel return $this), properties stay public.

The last submitted patch, 6: 2030613-EntityViewMode-protected.patch, failed testing.

The last submitted patch, 9: 2030613-EntityViewMode-protected-3.patch, failed testing.

The last submitted patch, 7: 2030613-EntityViewMode-2.patch, failed testing.

Berdir’s picture

10: 2030613-EntityViewMode-4.patch queued for re-testing.

balagan’s picture

Assigned: balagan » Unassigned
fago’s picture

Status: Needs review » Needs work

We should move all method to the parent base class, where the properties are defined.

  1. +++ b/core/modules/entity/lib/Drupal/entity/Entity/EntityViewMode.php
    @@ -55,5 +55,18 @@
    +    return $this;;
    

    ONe comma too much.

  2. +++ b/core/modules/entity/lib/Drupal/entity/Entity/EntityViewMode.php
    @@ -55,5 +55,18 @@
    +  }
    +  /**
    

    Missing a new line.

  3. +++ b/core/modules/entity/lib/Drupal/entity/EntityViewModeInterface.php
    @@ -11,5 +11,22 @@
    +   * @param string targetType
    

    Should be string $target_type - also the parameter name needs to be changed.

  4. +++ b/core/modules/entity/lib/Drupal/entity/EntityViewModeInterface.php
    @@ -11,5 +11,22 @@
    +   *   The entity type that this EntityView is for
    

    No full sentence (misses trailing point).

  5. +++ b/core/modules/entity/lib/Drupal/entity/EntityViewModeInterface.php
    @@ -11,5 +11,22 @@
    +   * @return self
    

    return $this.

  6. +++ b/core/modules/entity/lib/Drupal/entity/EntityViewModeInterface.php
    @@ -11,5 +11,22 @@
    +  public function setTargetType($targetType);
    

    So let's rename the getter and the setter to go with the "getTargetEntityTypeId" naming, to be consistent with FieldConfig and inline with the entity property name.

  7. +++ b/core/modules/entity/lib/Drupal/entity/EntityViewModeInterface.php
    @@ -11,5 +11,22 @@
    +   *   The label to be set for the viewMode.
    

    no camel casing here.

  8. +++ b/core/modules/entity/lib/Drupal/entity/EntityViewModeInterface.php
    @@ -11,5 +11,22 @@
    +   * @return self
    

    should be return $this

balagan’s picture

Assigned: Unassigned » balagan
balagan’s picture

Tried to implement the changes recommended by fago in this patch.

balagan’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 18: 2030613-EntityViewMode-17.patch, failed testing.

balagan’s picture

The test failed, I have tried to fix the call to undefined method problems in this patch. Anyone is welcome to commend on the other errors warnings, here: https://qa.drupal.org/pifr/test/755168
I don't really get anything else except the mentioned undefined methods.

balagan’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 21: 2030613-EntityViewMode-21.patch, failed testing.

balagan’s picture

Trying again.

balagan’s picture

Status: Needs work » Needs review
fago’s picture

  1. +++ b/core/modules/entity/lib/Drupal/entity/EntityDisplayModeInterface.php
    @@ -20,6 +20,26 @@
    +    /**
    

    Code style

  2. +++ b/core/modules/entity/lib/Drupal/entity/EntityDisplayModeInterface.php
    @@ -20,6 +20,26 @@
    +   * @param string target_type
    +   *   The entity type that this EntityView is for.
    

    Parameter should be target type id.

  3. +++ b/core/modules/entity/lib/Drupal/entity/EntityDisplayModeInterface.php
    @@ -20,6 +20,26 @@
    +   * @return this
    

    return $this

  4. +++ b/core/modules/entity/lib/Drupal/entity/EntityDisplayModeInterface.php
    @@ -20,6 +20,26 @@
    +   *   The label to be set for the view mode.
    

    display mode.

This needs an issue summary mentioning the API changes of the target entity type method (use the issue summary template).

fago’s picture

Status: Needs review » Needs work
balagan’s picture

Trying again. Also changed the parameter to target_type_id in the other file.

balagan’s picture

Assigned: balagan » Unassigned
Status: Needs work » Needs review
fago’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/entity/lib/Drupal/entity/EntityDisplayModeBase.php
    @@ -66,8 +66,8 @@ public static function sort(ConfigEntityInterface $a, ConfigEntityInterface $b)
    +    $a_type = $a->getTargetTypeId();
    

    As discussed, it should be getTargetEntityTypeId() to be consisitent with FieldConfig. Everywhere here + the setter.

  2. +++ b/core/modules/entity/lib/Drupal/entity/EntityDisplayModeInterface.php
    @@ -20,6 +20,26 @@
    +   *   The entity type that this EntityView is for.
    

    The entity type to be displayed.

balagan’s picture

Trying to fix the errors.
I am whether name should be mentioned in the comments when the function is called getTargetEntityTypeId, and supposedly returns an id.

  /**
   * Returns the entity type this display mode is used for.
   *
   * @return string
   *   The entity type name.
   */
  public function getTargetEntityTypeId();
balagan’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 31: 2030613-EntityViewMode-31.patch, failed testing.

balagan’s picture

Missed some methodnames in the previous patch. This patch passed tests on my localhost.

balagan’s picture

Status: Needs work » Needs review
fago’s picture

Status: Needs review » Needs work

Looks good, thanks. This should include a small test to make sure the newly added setter works though. Besides that we need an issue summary that lists the API change of the target entity type getter.

balagan’s picture

Issue summary: View changes
balagan’s picture

I have updated the issue summary with the API changes. If that is acceptable, and only the writing of the test is needed, then I think the novice tag should be removed from this issue.

Xano’s picture

Status: Needs work » Needs review
balagan’s picture

I have set the issue to needs work, because @fago said in comment #36 that some tests are needed to make sure setters are working properly.

balagan’s picture

Issue summary: View changes
Xano’s picture

Issue tags: +Needs tests
alexpott’s picture

Issue tags: -Needs tests
FileSize
7.61 KB

PSR-4'd, made properties protected and added tests.

Status: Needs review » Needs work

The last submitted patch, 43: 2030613.43.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
808 bytes
8.66 KB

Fixed tests

scottrigby’s picture

Assigned: Unassigned » scottrigby
Issue summary: View changes
Status: Needs review » Needs work
Issue tags: -Novice +Needs reroll
curl https://www.drupal.org/files/issues/2030613.45.patch | git apply
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  8867  100  8867    0     0   6985      0  0:00:01  0:00:01 --:--:--  6987
error: patch failed: core/lib/Drupal/Core/Entity/EntityManager.php:813
error: core/lib/Drupal/Core/Entity/EntityManager.php: patch does not apply
error: patch failed: core/modules/entity/src/EntityDisplayModeBase.php:48
error: core/modules/entity/src/EntityDisplayModeBase.php: patch does not apply

Updated issue summary & working on reroll.

scottrigby’s picture

scottrigby’s picture

scottrigby’s picture

Issue summary: View changes
Issue tags: -Needs reroll
FileSize
7.65 KB

Applied with curl 2030613.45.patch | git apply --3.

Attaching the reroll with no changes.

Also, #2030613: Expand EntityViewMode (really EntityDisplayModeBase) with methods was since committed, so will add a new patch with interdiff to remove that next.

scottrigby’s picture

Assigned: scottrigby » Unassigned
Status: Needs work » Needs review
FileSize
7.65 KB
548 bytes

Only removed TODO (interdiff attached).

Since I rerolled this, someone else should review.

scottrigby’s picture

BTW the patch looked perfect to me while reviewing. I didn't test it, because in reviewing the automated tests added did the exact same steps I would have gone through, and they pass. I didn't mark RTBC only because I rerolled.

MKorostoff’s picture

Status: Needs review » Reviewed & tested by the community

Confirming that the re-roll in #50 applies cleanly

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
--- /dev/null
+++ b/.idea/scopes/scope_settings.xml

+++ b/.idea/scopes/scope_settings.xml
@@ -0,0 +1,5 @@

@@ -0,0 +1,5 @@
+<component name="DependencyValidationManager">
+  <state>
+    <option name="SKIP_IMPORT_STATEMENTS" value="false" />
+  </state>
+</component>

the PHPstorm file shouldn't be part of the patch

scottrigby’s picture

Status: Needs work » Needs review
FileSize
7.3 KB

:facepalm: IntelliJ file removed!

Berdir queued 54: 2030613.54.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 54: 2030613.54.patch, failed testing.

Mile23’s picture

Status: Needs work » Needs review
FileSize
7.38 KB

Reroll of #54, with 3-way merge happiness.

Status: Needs review » Needs work

The last submitted patch, 57: 2030613_57.patch, failed testing.

Mile23’s picture

Status: Needs work » Needs review
FileSize
13.28 KB
1.35 KB

Unit tests were mocking a class that had been moved.

Status: Needs review » Needs work

The last submitted patch, 59: 2030613_59.patch, failed testing.

alexpott’s picture

index c5aae17..76aafd0 100644
--- a/core/includes/menu.inc

--- a/core/includes/menu.inc
+++ b/core/includes/menu.inc

index c0301e1..cbbac44 100644
--- a/core/modules/entity_reference/src/ConfigurableEntityReferenceItem.php

--- a/core/modules/entity_reference/src/ConfigurableEntityReferenceItem.php
+++ b/core/modules/entity_reference/src/ConfigurableEntityReferenceItem.php

index 265651c..3a32d69 100644
--- a/core/modules/entity_reference/src/Plugin/Field/FieldFormatter/EntityReferenceFormatterBase.php

--- a/core/modules/entity_reference/src/Plugin/Field/FieldFormatter/EntityReferenceFormatterBase.php
+++ b/core/modules/entity_reference/src/Plugin/Field/FieldFormatter/EntityReferenceFormatterBase.php

The changes to these files are not this patch afaics

Mile23’s picture

Yah, I literally pulled 8.0.x, made this patch, and before I uploaded it the files moved. Will reroll.

Mile23’s picture

Status: Needs work » Needs review
FileSize
7.39 KB

EZ re-roll.

alexpott’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/Entity/EntityDisplayModeBase.php
    @@ -74,16 +74,32 @@ public static function sort(ConfigEntityInterface $a, ConfigEntityInterface $b)
    -  public function getTargetType() {
    +  public function getTargetEntityTypeId() {
    

    This is an API change and, whilst it is better, it is not so much better that we should break our API change policy. So let's rename setTargetEntityTypeId to setTargetType. If you feel strongly that this is confusing then we should deprecate getTargetType and add the new getTargetEntityTypeId() and setTargetEntityTypeId methods.

  2. +++ b/core/lib/Drupal/Core/Entity/EntityDisplayModeBase.php
    @@ -74,16 +74,32 @@ public static function sort(ConfigEntityInterface $a, ConfigEntityInterface $b)
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function setLabel($label) {
    +    $this->set('label', $label);
    +    return $this;
    +  }
    +
    

    There are no usages outside of tests so let's not add this.

EDIT: For clarity

Mile23’s picture

Title: Expand EntityViewMode with methods » Expand EntityViewMode (really EntityDisplayModeBase) with methods
Status: Needs work » Needs review
FileSize
1.2 KB

OK, so a couple of things here:

1) The reroll includes all the work done around setTargetEntityTypeId(), which is apparently @Fago-approved (#30), and which @alexpott worked on in #45. :-) But I get that it might be an API change during the alpha cycle, so maybe that's a reason to hold back on it.

2) I was told at a sprint and in IRC by @alexpott and @yesct that this issues and it's siblings from #2016679: Expand Entity Type interfaces to provide methods, protect the properties had a scope change to simply make all properties protected and add getters and setters as needed.

So with #2 in mind, here's a patch to EntityDisplayModeBase which only marks those properties as protected, and then calls it a day. Existing tests should fail if this accessibility change means anything to existing code, and obviously setting them protected signals developers not to use them moving forward.

I couldn't get local tests to fail, so let's see what the testbot says.

If this strategy is a bad one, we can get back to messing around with the patch in #63.

Status: Needs review » Needs work

The last submitted patch, 65: 2030613_65_protected_properties_only.patch, failed testing.

Mile23’s picture

FileSize
4.73 KB
5.67 KB

Expanding on the patch in #65, and test results, here's what I did:

  1. Added setTargetType() to EntityDisplayModeInterface. This mirrors the existing getTargetType(). EntityDisplayModeInterface is only implemented by two other classes, and neither overrides any relevant methods from EntityDisplayModeBase.
  2. Replaced direct access with setters in core/modules/field_ui/src/Form/EntityDisplayModeAddForm.php and core/modules/field_ui/src/Form/EntityFormModeAddForm.php.
  3. Updated unit tests for getter and setter.
Mile23’s picture

Status: Needs work » Needs review
daffie’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityDisplayModeBase.php
@@ -20,14 +20,14 @@
    *
    * @var string
    */
-  public $id;
+  protected $id;
 
   /**
    * The human-readable name of the form or view mode.
    *
    * @var string
    */
-  public $label;
+  protected $label;
 

The variables $id and $label do not have to be declared here. They are inherited from the Entity class.

+++ b/core/lib/Drupal/Core/Entity/EntityDisplayModeBase.php
@@ -81,6 +81,14 @@ public function getTargetType() {
+    $this->targetEntityType = $target_entity_type;
+    return $this;

Should we not use the set function for this.

    $this->set('targetEntityType', $target_entity_type);
daffie’s picture

Status: Needs review » Needs work
Mile23’s picture

Actually, $id and $label are not provided by Entity. Entity has ::id() and ::label() methods which bend over backwards to avoid storing their information at that level.

Also, since $targetEntityType is provided by this class, it makes more sense to just set it, unless there are some undocumented side-effects provided by set() that we need. Looking at set(), we see that it might need to set the value in a plugin collection.

I'm not sure if this base class is supposed to care about that.

daffie’s picture

@Mile23: Yes, you are right. My mistake. The variables $id and $label must be declared.

Some variables do use the set(). And others do not. I have no idea what to do.

Status: Needs work » Needs review

daffie queued 67: 2030613_67.patch for re-testing.

daffie’s picture

I have reviewed the patch and it all looks good to me.
The patch is a month old so to be sure I have it retested.
The class variables are protected and the function setTargetType() is added.
There are PHPUnit tests added.
The documentation is also in order.
If retesting gives a green result I shall give the issue a RTBC.

daffie’s picture

Status: Needs review » Reviewed & tested by the community

The re-test gives all green. So RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Whilst I worked on one version of this patch @Mile23 rewrote this patch in #65 so I think that me committing this is fine. This issue is a prioritized change (see #2016679: Expand Entity Type interfaces to provide methods, protect the properties) as per https://www.drupal.org/core/beta-changes and it's benefits outweigh any disruption. Committed 3827a48 and pushed to 8.0.x. Thanks!

  • alexpott committed 3827a48 on 8.0.x
    Issue #2030613 by balagan, Mile23, scottrigby, alexpott: Expand...

Status: Fixed » Closed (fixed)

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