EntityForm has a magic setter and getter to take care of triggering a deprecation error, eg:

  public function __get($name) {
    // Removing core's usage of ::setEntityManager means that this deprecated
    // service wont be set. We provide it here for backwards compatibility.
    if ($name === 'entityManager') {
      @trigger_error('EntityForm::entityManager is deprecated in drupal:8.0.0 and is removed from drupal:9.0.0. Use EntityForm::entityTypeManager instead. See https://www.drupal.org/node/2549139', E_USER_DEPRECATED);
      return $this->privateEntityManager ?: \Drupal::entityManager();
    }
  }

However, this does nothing when $name is not entityManager. So any form class that does $this->foo = 'bar' without having declared $foo as a class property will suddenly find that setting the value has no effect.

Credit to @aalin to figuring this one at #3093329: $moduleEntityData error in ComponentSectionForm.php.

CommentFileSizeAuthor
#21 20-21-interdiff.txt1.04 KBalexpott
#21 3094913-21.patch1.41 KBalexpott
#20 3094913-21.patch1.87 KBalexpott
#20 3094913-21.test-only.patch719 bytesalexpott
#20 15-21-interdiff.txt1020 bytesalexpott
#17 interdiff_15-17.txt997 bytesakashkumar07
#17 3094913-17.patch1.16 KBakashkumar07
#15 3094913-15.patch1.16 KBneel24
#13 interdiff_8-13.txt1.41 KBakashkumar07
#13 3094913-13.patch1.14 KBakashkumar07
#10 interdiff-3094913-4-10.txt1.45 KBhardik_patel_12
#10 3094913-10.patch1.18 KBhardik_patel_12
#8 interdiff-4-8.txt764 bytesneel24
#8 3094913-8.patch1.63 KBneel24
#4 3094913-4.patch1.64 KBjoelpittet

Comments

joachim created an issue. See original summary.

joachim’s picture

joachim commented
Issue summary: View changes
joachim’s picture

joachim commented
Issue summary: View changes
joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Active » Needs review
Related issues: +#3097934: Fatal error on admin page
StatusFileSize
new 3094913-4.patch1.64 KB

How about something like this?

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Parent issue: » #3052233: Properly deprecate EntityFormInterface::setEntityManager() and trigger an error on use

Adding follow-up from parent

joachim’s picture

joachim commented 
+++ b/core/lib/Drupal/Core/Entity/EntityForm.php
@@ -68,6 +80,9 @@ public function __get($name) {
+      return $this->privateData[$name];

I don't understand we need the intermediary of the privateData array.

Can we not just do $this->{$name}?

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work

Yes, I think we can just check for/set $this->$name. There's also a typo to fix in __set(): "This *is* property".

neel24’s picture

neel24 commented
Status: Needs work » Needs review
StatusFileSize
new 3094913-8.patch1.63 KB
new interdiff-4-8.txt764 bytes

I made some changes based on comment #7. Attaching an interdiff as well.

agrochal’s picture

agrochal commented
Status: Needs review » Needs work

In the "__set" function, there's still $this->privateData[$name] = value;. I think we can also remove declaration of this array private $privateData = [];.

hardik_patel_12’s picture

hardik_patel_12 commented
Status: Needs work » Needs review
StatusFileSize
new 3094913-10.patch1.18 KB
new interdiff-3094913-4-10.txt1.45 KB

Changes based on comment #4 , #7 and #9. Attaching an interdiff as well.

joachim’s picture

joachim commented
Status: Needs review » Needs work

Looking good, just the comment needs work:

+++ b/web/core/lib/Drupal/Core/Entity/EntityForm.php
@@ -82,6 +85,11 @@ public function __set($name, $value) {
+      // This is property exists because this magic method breaks when

This sentence doesn't read properly.

Madhura BK’s picture

Madhura BK commented
Assigned: Unassigned » Madhura BK
akashkumar07’s picture

akashkumar07 commented
Assigned: Madhura BK » Unassigned
Status: Needs work » Needs review
StatusFileSize
new 3094913-13.patch1.14 KB
new interdiff_8-13.txt1.41 KB

Removed declaration of 

 array private $privateData = [];
hardik_patel_12’s picture

hardik_patel_12 commented
Status: Needs review » Needs work

@AkashkumarOSL ,

Kindly look comment #11 and #10 , we need to change comments only now in set method.

neel24’s picture

neel24 commented
Status: Needs work » Needs review
StatusFileSize
new 3094913-15.patch1.16 KB

I've created a new patch based on the necessary changes.

joachim’s picture

joachim commented
Status: Needs review » Reviewed & tested by the community

LGTM.

Thanks!

akashkumar07’s picture

akashkumar07 commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new 3094913-17.patch1.16 KB
new interdiff_15-17.txt997 bytes

Please review this patch.

hardik_patel_12’s picture

hardik_patel_12 commented
Status: Needs review » Reviewed & tested by the community

Patch written in #15 looks great to me.
@AkashkumarOSL , changes done in #17's patch is not required kindly look patch written in #15.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

Yep #17 is an incorrect bugfix. #15 looks like the correct fix but #17 shows that we don't have any test coverage. We should have that.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Needs review
StatusFileSize
new 15-21-interdiff.txt1020 bytes
new 3094913-21.test-only.patch719 bytes
new 3094913-21.patch1.87 KB

Actually there was a bug in #15 too. Here's a test and fix.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
StatusFileSize
new 3094913-21.patch1.41 KB
new 20-21-interdiff.txt1.04 KB 
+++ b/core/lib/Drupal/Core/Entity/EntityForm.php
@@ -68,6 +68,9 @@ public function __get($name) {
+    elseif (isset($this->$name)) {
+      return $this->$name;
+    }

This is not necessary.

The last submitted patch, 20: 3094913-21.test-only.patch, failed testing. View results

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Reviewed & tested by the community

Looks good.

Note: This BC layer was already removed in 9.0.x, so this does not need to be committed there.

  • catch committed 7d7c475 on 8.9.x 
    Issue #3094913 by alexpott, AkashkumarOSL, neel24, Hardik_Patel_12,...

  • catch committed 5521e96 on 8.8.x 
    Issue #3094913 by alexpott, AkashkumarOSL, neel24, Hardik_Patel_12,...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed 7d7c475 and pushed to 8.9.x, then cherry-picked to 8.8.x. Thanks!

Status: Fixed » Closed (fixed)

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