Problem/Motivation

Once we can use property hooks, we can define entity as a property on EntityReferenceFieldItemList and use property hooks to return the referenced entity.

This will allow us to completely remove the hack added in #3565937: Workaround PHP bug with fibers and __get() because property hooks don't have the same guarding issue.

The test coverage added in there can stay - since that demonstrates that the fiber suspend isn't broken by any PHP bugs.

Since the entity property is being converted to property hooks, it might make sense to do the target_id property here as well.

Also, per Zend blog, "isset operations will hit the 'get' hook", so the get property hook will fix #3590537: Workaround PHP bug with fibers and __isset() for D12 as well.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3566626

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

catch created an issue. See original summary.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

godotislate made their first commit to this issue’s fork.

godotislate opened merge request !14588

godotislate’s picture

godotislate
he/him
commented
Status: Active » Needs work

Took a shot with property hooks for entity in both EntityReferenceItem and EntityReferenceItemList. I chose EntityReferenceItem over EntityReferenceItemBase, since entity is defined in EntityReferenceItem::propertyDefinitions and not the code>EntityReferenceItemBase class.

I thought it also made sense to add property hooks for target_id, even though that property isn't involved in the Fiber issue.

But the test PHPCS and PHPStan configuration doesn't like property hooks:
https://git.drupalcode.org/issue/drupal-3566626/-/jobs/8312295

PHP_CodeSniffer version 3.13.5 (stable) by Squiz and PHPCSStandards
$ composer phpcs -- -s --report-full --report-summary --report-\\Micheh\\PhpCodeSniffer\\Report\\Gitlab=$_ARTIFACTS_DIR/phpcs-quality-report.json --cache=core/.phpcscache
FILE: .../core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
--------------------------------------------------------------------------------
FOUND 26 ERRORS AND 39 WARNINGS AFFECTING 56 LINES
--------------------------------------------------------------------------------
   1 | ERROR   | [ ] An error occurred during processing; checking has been
     |         |     aborted. The error message was: Undefined array key "" in
     |         |     /builds/vendor/slevomat/coding-standard/SlevomatCodingStandard/Helpers/TernaryOperatorHelper.php
     |         |     on line 59
     |         |     The error originated in the
     |         |     SlevomatCodingStandard.ControlStructures.RequireShortTernaryOperator
     |         |     sniff on line 47. (Internal.Exception)
  36 | ERROR   | [x] Multi-line function call not indented correctly; expected
     |         |     4 spaces but found 2
     |         |     (PEAR.Functions.FunctionCallSignature.Indent)
  37 | ERROR   | [x] Multi-line function call not indented correctly; expected
     |         |     4 spaces but found 2
     |         |     (PEAR.Functions.FunctionCallSignature.Indent)
  38 | ERROR   | [x] Multi-line function call not indented correctly; expected
     |         |     4 spaces but found 2
     |         |     (PEAR.Functions.FunctionCallSignature.Indent)
  39 | ERROR   | [x] Array value not aligned correctly; expected 16 spaces but
     |         |     found 4 (Squiz.Arrays.ArrayDeclaration.ValueNotAligned)
  40 | ERROR   | [x] Multi-line function call not indented correctly; expected
     |         |     4 spaces but found 2
     |         |     (PEAR.Functions.FunctionCallSignature.Indent)
  40 | ERROR   | [x] Closing parenthesis not aligned correctly; expected 15
     |         |     spaces but found 2
     |         |     (Squiz.Arrays.ArrayDeclaration.CloseBraceNotAligned)
  41 | ERROR   | [x] Multi-line function call not indented correctly; expected
     |         |     4 spaces but found 2
     |         |     (PEAR.Functions.FunctionCallSignature.Indent)
  42 | ERROR   | [x] Multi-line function call not indented correctly; expected
     |         |     4 spaces but found 2
     |         |     (PEAR.Functions.FunctionCallSignature.Indent)
  43 | ERROR   | [x] Multi-line function call not indented correctly; expected
     |         |     4 spaces but found 2
     |         |     (PEAR.Functions.FunctionCallSignature.Indent)
  44 | ERROR   | [x] Multi-line function call not indented correctly; expected
     |         |     4 spaces but found 2
     |         |     (PEAR.Functions.FunctionCallSignature.Indent)
  51 | ERROR   | [ ] There must not be more than one property declared per
     |         |     statement (PSR2.Classes.PropertyDeclaration.Multiple)
  52 | ERROR   | [x] Line indented incorrectly; expected 2 spaces, found 4
     |         |     (Drupal.WhiteSpace.ScopeIndent.IncorrectExact)
  53 | ERROR   | [x] Line indented incorrectly; expected 2 spaces, found 6
     |         |     (Drupal.WhiteSpace.ScopeIndent.IncorrectExact)
  53 | ERROR   | [ ] Missing member variable doc comment
     |         |     (Drupal.Commenting.VariableComment.Missing)
  53 | ERROR   | [ ] Visibility must be declared on property "$this"
     |         |     (PSR2.Classes.PropertyDeclaration.ScopeMissing)
  54 | ERROR   | [x] Line indented incorrectly; expected 2 spaces, found 4
     |         |     (Drupal.WhiteSpace.ScopeIndent.IncorrectExact)
  55 | WARNING | [ ] Code after the RETURN statement on line 53 cannot be
     |         |     executed (Squiz.PHP.NonExecutableCode.Unreachable)
  55 | ERROR   | [x] Line indented incorrectly; expected 2 spaces, found 4
     |         |     (Drupal.WhiteSpace.ScopeIndent.IncorrectExact)
  56 | WARNING | [ ] Code after the RETURN statement on line 53 cannot be
     |         |     executed (Squiz.PHP.NonExecutableCode.Unreachable)
  56 | ERROR   | [x] Line indented incorrectly; expected 2 spaces, found 6
     |         |     (Drupal.WhiteSpace.ScopeIndent.IncorrectExact)
  57 | ERROR   | [x] Line indented incorrectly; expected 2 spaces, found 4
     |         |     (Drupal.WhiteSpace.ScopeIndent.IncorrectExact)
  63 | WARNING | [ ] Code after the RETURN statement on line 53 cannot be
     |         |     executed (Squiz.PHP.NonExecutableCode.Unreachable)
  64 | WARNING | [ ] Code after the RETURN statement on line 53 cannot be
     |         |     executed (Squiz.PHP.NonExecutableCode.Unreachable)
  64 | ERROR   | [x] Line indented incorrectly; expected 2 spaces, found 4
     |         |     (Drupal.WhiteSpace.ScopeIndent.IncorrectExact)
  65 | WARNING | [ ] Code after the RETURN statement on line 53 cannot be
     |         |     executed (Squiz.PHP.NonExecutableCode.Unreachable)
  65 | ERROR   | [x] Line indented incorrectly; expected 2 spaces, found 6
     |         |     (Drupal.WhiteSpace.ScopeIndent.IncorrectExact)
  66 | ERROR   | [x] Line indented incorrectly; expected 2 spaces, found 4
     |         |     (Drupal.WhiteSpace.ScopeIndent.IncorrectExact)
  67 | WARNING | [ ] Code after the RETURN statement on line 53 cannot be
     |         |     executed (Squiz.PHP.NonExecutableCode.Unreachable)
  67 | ERROR   | [x] Line indented incorrectly; expected 2 spaces, found 4
     |         |     (Drupal.WhiteSpace.ScopeIndent.IncorrectExact)
  68 | WARNING | [ ] Code after the RETURN statement on line 53 cannot be
     |         |     executed (Squiz.PHP.NonExecutableCode.Unreachable)
  68 | ERROR   | [x] Line indented incorrectly; expected 2 spaces, found 6
     |         |     (Drupal.WhiteSpace.ScopeIndent.IncorrectExact)
  69 | ERROR   | [x] Line indented incorrectly; expected 2 spaces, found 4
     |         |     (Drupal.WhiteSpace.ScopeIndent.IncorrectExact)
  75 | WARNING | [ ] Code after the RETURN statement on line 53 cannot be
     |         |     executed (Squiz.PHP.NonExecutableCode.Unreachable)
  84 | WARNING | [ ] Code after the RETURN statement on line 53 cannot be
     |         |     executed (Squiz.PHP.NonExecutableCode.Unreachable)
  94 | WARNING | [ ] Code after the RETURN statement on line 53 cannot be
     |         |     executed (Squiz.PHP.NonExecutableCode.Unreachable)
 142 | WARNING | [ ] Code after the RETURN statement on line 53 cannot be
     |         |     executed (Squiz.PHP.NonExecutableCode.Unreachable)
 157 | WARNING | [ ] Code after the RETURN statement on line 53 cannot be
     |         |     executed (Squiz.PHP.NonExecutableCode.Unreachable)
 184 | WARNING | [ ] Code after the RETURN statement on line 53 cannot be
     |         |     executed (Squiz.PHP.NonExecutableCode.Unreachable)
 191 | WARNING | [ ] Code after the RETURN statement on line 53 cannot be
     |         |     executed (Squiz.PHP.NonExecutableCode.Unreachable)
 238 | WARNING | [ ] Code after the RETURN statement on line 53 cannot be
     |         |     executed (Squiz.PHP.NonExecutableCode.Unreachable)
 253 | WARNING | [ ] Code after the RETURN statement on line 53 cannot be
     |         |     executed (Squiz.PHP.NonExecutableCode.Unreachable)
 297 | WARNING | [ ] Code after the RETURN statement on line 53 cannot be
     |         |     executed (Squiz.PHP.NonExecutableCode.Unreachable)
 311 | WARNING | [ ] Code after the RETURN statement on line 53 cannot be
     |         |     executed (Squiz.PHP.NonExecutableCode.Unreachable)
 327 | WARNING | [ ] Code after the RETURN statement on line 53 cannot be
     |         |     executed (Squiz.PHP.NonExecutableCode.Unreachable)
 341 | WARNING | [ ] Code after the RETURN statement on line 53 cannot be
     |         |     executed (Squiz.PHP.NonExecutableCode.Unreachable)
 359 | WARNING | [ ] Code after the RETURN statement on line 53 cannot be
     |         |     executed (Squiz.PHP.NonExecutableCode.Unreachable)
 425 | WARNING | [ ] Code after the RETURN statement on line 53 cannot be
     |         |     executed (Squiz.PHP.NonExecutableCode.Unreachable)
 440 | WARNING | [ ] Code after the RETURN statement on line 53 cannot be
     |         |     executed (Squiz.PHP.NonExecutableCode.Unreachable)
 467 | WARNING | [ ] Code after the RETURN statement on line 53 cannot be
     |         |     executed (Squiz.PHP.NonExecutableCode.Unreachable)
 542 | WARNING | [ ] Code after the RETURN statement on line 53 cannot be
     |         |     executed (Squiz.PHP.NonExecutableCode.Unreachable)
 558 | WARNING | [ ] Code after the RETURN statement on line 53 cannot be
     |         |     executed (Squiz.PHP.NonExecutableCode.Unreachable)
 565 | WARNING | [ ] Code after the RETURN statement on line 53 cannot be
     |         |     executed (Squiz.PHP.NonExecutableCode.Unreachable)
 605 | WARNING | [ ] Code after the RETURN statement on line 53 cannot be
     |         |     executed (Squiz.PHP.NonExecutableCode.Unreachable)
 615 | WARNING | [ ] Code after the RETURN statement on line 53 cannot be
     |         |     executed (Squiz.PHP.NonExecutableCode.Unreachable)
 674 | WARNING | [ ] Code after the RETURN statement on line 53 cannot be
     |         |     executed (Squiz.PHP.NonExecutableCode.Unreachable)
 681 | WARNING | [ ] Code after the RETURN statement on line 53 cannot be
     |         |     executed (Squiz.PHP.NonExecutableCode.Unreachable)
 688 | WARNING | [ ] Code after the RETURN statement on line 53 cannot be
     |         |     executed (Squiz.PHP.NonExecutableCode.Unreachable)
 698 | WARNING | [ ] Code after the RETURN statement on line 53 cannot be
     |         |     executed (Squiz.PHP.NonExecutableCode.Unreachable)
 724 | WARNING | [ ] Code after the RETURN statement on line 53 cannot be
     |         |     executed (Squiz.PHP.NonExecutableCode.Unreachable)
 734 | WARNING | [ ] Code after the RETURN statement on line 53 cannot be
     |         |     executed (Squiz.PHP.NonExecutableCode.Unreachable)
 757 | WARNING | [ ] Code after the RETURN statement on line 53 cannot be
     |         |     executed (Squiz.PHP.NonExecutableCode.Unreachable)
 769 | WARNING | [ ] Code after the RETURN statement on line 53 cannot be
     |         |     executed (Squiz.PHP.NonExecutableCode.Unreachable)
 778 | WARNING | [ ] Code after the RETURN statement on line 53 cannot be
     |         |     executed (Squiz.PHP.NonExecutableCode.Unreachable)
 809 | WARNING | [ ] Code after the RETURN statement on line 53 cannot be
     |         |     executed (Squiz.PHP.NonExecutableCode.Unreachable)
--------------------------------------------------------------------------------
PHPCBF CAN FIX THE 22 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------
FILE: /builds/core/lib/Drupal/Core/Field/EntityReferenceFieldItemList.php
--------------------------------------------------------------------------------
FOUND 16 ERRORS AND 11 WARNINGS AFFECTING 19 LINES
--------------------------------------------------------------------------------
   1 | ERROR   | [ ] An error occurred during processing; checking has been
     |         |     aborted. The error message was: Undefined array key "" in
     |         |     /builds/vendor/slevomat/coding-standard/SlevomatCodingStandard/Helpers/TernaryOperatorHelper.php
     |         |     on line 59
     |         |     The error originated in the
     |         |     SlevomatCodingStandard.ControlStructures.RequireShortTernaryOperator
     |         |     sniff on line 47. (Internal.Exception)
  17 | ERROR   | [ ] There must not be more than one property declared per
     |         |     statement (PSR2.Classes.PropertyDeclaration.Multiple)
  18 | ERROR   | [x] Line indented incorrectly; expected 2 spaces, found 4
     |         |     (Drupal.WhiteSpace.ScopeIndent.IncorrectExact)
  19 | ERROR   | [x] Line indented incorrectly; expected 2 spaces, found 6
     |         |     (Drupal.WhiteSpace.ScopeIndent.IncorrectExact)
  19 | ERROR   | [ ] Missing member variable doc comment
     |         |     (Drupal.Commenting.VariableComment.Missing)
  19 | ERROR   | [ ] Visibility must be declared on property "$this"
     |         |     (PSR2.Classes.PropertyDeclaration.ScopeMissing)
  20 | ERROR   | [x] Line indented incorrectly; expected 2 spaces, found 4
     |         |     (Drupal.WhiteSpace.ScopeIndent.IncorrectExact)
  21 | WARNING | [ ] Code after the RETURN statement on line 19 cannot be
     |         |     executed (Squiz.PHP.NonExecutableCode.Unreachable)
  21 | ERROR   | [x] Line indented incorrectly; expected 2 spaces, found 4
     |         |     (Drupal.WhiteSpace.ScopeIndent.IncorrectExact)
  22 | WARNING | [ ] Code after the RETURN statement on line 19 cannot be
     |         |     executed (Squiz.PHP.NonExecutableCode.Unreachable)
  22 | ERROR   | [x] Line indented incorrectly; expected 2 spaces, found 6
     |         |     (Drupal.WhiteSpace.ScopeIndent.IncorrectExact)
  23 | ERROR   | [x] Line indented incorrectly; expected 2 spaces, found 4
     |         |     (Drupal.WhiteSpace.ScopeIndent.IncorrectExact)
  29 | WARNING | [ ] Code after the RETURN statement on line 19 cannot be
     |         |     executed (Squiz.PHP.NonExecutableCode.Unreachable)
  30 | WARNING | [ ] Code after the RETURN statement on line 19 cannot be
     |         |     executed (Squiz.PHP.NonExecutableCode.Unreachable)
  30 | ERROR   | [x] Line indented incorrectly; expected 2 spaces, found 4
     |         |     (Drupal.WhiteSpace.ScopeIndent.IncorrectExact)
  31 | WARNING | [ ] Code after the RETURN statement on line 19 cannot be
     |         |     executed (Squiz.PHP.NonExecutableCode.Unreachable)
  31 | ERROR   | [x] Line indented incorrectly; expected 2 spaces, found 6
     |         |     (Drupal.WhiteSpace.ScopeIndent.IncorrectExact)
  32 | ERROR   | [x] Line indented incorrectly; expected 2 spaces, found 4
     |         |     (Drupal.WhiteSpace.ScopeIndent.IncorrectExact)
  33 | WARNING | [ ] Code after the RETURN statement on line 19 cannot be
     |         |     executed (Squiz.PHP.NonExecutableCode.Unreachable)
  33 | ERROR   | [x] Line indented incorrectly; expected 2 spaces, found 4
     |         |     (Drupal.WhiteSpace.ScopeIndent.IncorrectExact)
  34 | WARNING | [ ] Code after the RETURN statement on line 19 cannot be
     |         |     executed (Squiz.PHP.NonExecutableCode.Unreachable)
  34 | ERROR   | [x] Line indented incorrectly; expected 2 spaces, found 6
     |         |     (Drupal.WhiteSpace.ScopeIndent.IncorrectExact)
  35 | ERROR   | [x] Line indented incorrectly; expected 2 spaces, found 4
     |         |     (Drupal.WhiteSpace.ScopeIndent.IncorrectExact)
  41 | WARNING | [ ] Code after the RETURN statement on line 19 cannot be
     |         |     executed (Squiz.PHP.NonExecutableCode.Unreachable)
  51 | WARNING | [ ] Code after the RETURN statement on line 19 cannot be
     |         |     executed (Squiz.PHP.NonExecutableCode.Unreachable)
  87 | WARNING | [ ] Code after the RETURN statement on line 19 cannot be
     |         |     executed (Squiz.PHP.NonExecutableCode.Unreachable)
 132 | WARNING | [ ] Code after the RETURN statement on line 19 cannot be
     |         |     executed (Squiz.PHP.NonExecutableCode.Unreachable)
--------------------------------------------------------------------------------

https://git.drupalcode.org/issue/drupal-3566626/-/jobs/8312294

-- --------------------------------------------------------------------------- 
     Error                                                                      
 -- --------------------------------------------------------------------------- 
     Internal error: Method                                                     
     Drupal\Core\Field\Plugin\Field\FieldType\EntityReferenceItem::$entity::ge  
     t() does not exist while analysing file                                    
     /builds/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReference  
     Item.php                                                                   
                                                                                
     Run PHPStan with -v option and post the stack trace to:                    
     https://github.com/phpstan/phpstan/issues/new?template=Bug_report.yaml     
                                                                                
     Internal error: Method                                                     
     Drupal\Core\Field\EntityReferenceFieldItemList::$target_id::get() does     
     not exist while analysing file                                             
     /builds/core/lib/Drupal/Core/Field/EntityReferenceFieldItemList.php        
                                                                                
     Run PHPStan with -v option and post the stack trace to:                    
     https://github.com/phpstan/phpstan/issues/new?template=Bug_report.yaml     
                                                                                
 -- ---------------------------------------------------------------------------

I ignored these temporarily, and otherwise tests are passing.

godotislate’s picture

godotislate
he/him
commented

Not sure about PHPStan, but looks like PHPCS support for property hooks is still in progress: https://github.com/PHPCSStandards/PHP_CodeSniffer/issues/734

mondrake’s picture

mondrake
Location 🇮🇹
commented

Got similar PHPStan error while playing with property hooks in the image system:

 -- ------------------------------------------------------------------------------------------------------------------------------------------------- 
     Error                                                                                                                                            
 -- ------------------------------------------------------------------------------------------------------------------------------------------------- 
     Internal error: Method Drupal\system\Plugin\ImageToolkit\Operation\gd\Scale::$needsToolkitInjection::get() does not exist while analysing file   
     /Users/xxxxx/Dev/drupal/core/modules/system/src/Plugin/ImageToolkit/Operation/gd/Scale.php                                             
                                                                                                                                                      
     Run PHPStan with -v option and post the stack trace to:                                                                                          
     https://github.com/phpstan/phpstan/issues/new?template=Bug_report.yaml                                                                           
                                                                                                                                                      
     Internal error: Method Drupal\system\Plugin\ImageToolkit\Operation\gd\Resize::$needsToolkitInjection::get() does not exist while analysing file  
     /Users/xxxxx/Dev/drupal/core/modules/system/src/Plugin/ImageToolkit/Operation/gd/Resize.php                                            
                                                                                                                                                      
     Run PHPStan with -v option and post the stack trace to:                                                                                          
     https://github.com/phpstan/phpstan/issues/new?template=Bug_report.yaml                                                                           
                                                                                                                                                      
 -- -------------------------------------------------------------------------------------------------------------------------------------------------

if I composer remove mglaman/phpstan-drupal, the error disappears. Not necessarily a phpstan-drupal issue, anyway - could also be related to phpstan-deprecation-rules.

Looks like we're having a general problem with property hooks atm.

mondrake’s picture

mondrake
Location 🇮🇹
commented

#7 the issue is in \mglaman\PHPStanDrupal\DeprecatedScope\IgnoreDeprecationsScope. That method tries to reflect property hook g/setter as if they were normal methods which they aren't apparently.

mondrake’s picture

mondrake
Location 🇮🇹
commented

I proposed a PR with a fix upstream, https://github.com/mglaman/phpstan-drupal/pull/935

godotislate’s picture

godotislate
he/him
commented

Thanks for investigating!
I think the bigger challenge might be PHPCS, but one step at a time.

quietone’s picture

quietone commented
Title: [12.x] Use property hooks for EntityReferenceFieldItemList->entity » Use property hooks for EntityReferenceFieldItemList->entity
godotislate’s picture

godotislate
he/him
commented
Status: Needs work » Needs review

Rebased for conflict.

I'll push this forward to NR to see how we feel about going forward with PHPCS disabled on the file.

godotislate’s picture

godotislate
he/him
commented
Issue summary: View changes
berdir’s picture

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

Reviewed.

godotislate’s picture

godotislate
he/him
commented
Status: Needs work » Needs review

Made the requested changes. I'll confirm whether the property hooks resolve the isset thing for #3590537: Workaround PHP bug with fibers and __isset() later, but PHP documentation suggests it should.

berdir’s picture

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

I verified that this fixes the test added by that issue, very nice.

Feels like this is a first step toward making the entity field API a little bit more sensible, if slightly awkward due to the phpcs issues.

My understanding is that we can't backport this as it requires PHP 8.4, but I'll comment on the other issue that we only need to add that on D11 then.

berdir’s picture

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

Sorry, only realized now that the EntityReferenceFieldItemList also needs to change to __get()

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

And, do we want to add the explicit test coverage from the other issue, or do we rely on the phpcs docs which say it's covered?

godotislate’s picture

godotislate
he/him
commented
Title: Use property hooks for EntityReferenceFieldItemList->entity » Use property hooks for entity reference item and list entity and target_id properties
Issue summary: View changes
Status: Needs work » Needs review

OK, switched to using item->__get in the list class. I did this for both target_id and entity, because calling __get() directly does not cause the fiber bug, as seen by the tests still passing.

I reverted the __set change in the item, because __set in FieldItemBase looks like this:

  public function __set($name, $value) {
    // Support setting values via property objects, but take care in as the
    // value of the 'entity' property is typed data also.
    if ($value instanceof TypedDataInterface && !($value instanceof EntityInterface)) {
      $value = $value->getValue();
    }
    $this->set($name, $value);
  }

For the target_id and entity properties, this is no different from calling set() directly.

Also added the test from #3590537: Workaround PHP bug with fibers and __isset() to the MR here.

berdir’s picture

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

Lets do this. See review thread on discussions around how to implement this exactly and possible edge cases around BC. Since this applies to code that previous went to __get()/__set() magic methods, there's a BC break on what I think is an edge case, but it was supported before. There is no explicit test coverage for that in core, not for these properties at least.

This is a major only change change, I created a CR for it, we could possible formally deprecate it for all properties in an attempt to reduce all the possible combinations of ways you get and set field values, which I think is really needed.

  • catch committed e24cd797 on main 
    task: #3566626 Use property hooks for entity reference item and list...
catch’s picture

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

This looks great. It's a shame about phpcs, the author pointed out the problems supporting it before the rfc went in at all and was ignored, hopefully support will get worked out eventually.

But being able to remove the Fiber workaround makes this more than well worth it, as well as the overall simplification. I think we'll be able to do more along the same lines too.

Committed/pushed to main, thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

amateescu’s picture

amateescu commented

I haven't seen this mentioned in the issue, so.. are we ok with unset($item->target_id) throwing Error: Cannot unset hooked property? It's quite a BC break with no way to add a deprecation layer, unless we do it in \Drupal\Core\Field\FieldItemBase::__unset() for everyone.

godotislate’s picture

godotislate
he/him
commented

Do people do unset($item->target_id) commonly? We have a minor BC break already by not allowing entity or target_id to be set to typed data properties, so we could note the unset in the same CR, advising people to use $item->target_id = NULL.

It's quite a BC break with no way to add a deprecation layer, unless we do it in \Drupal\Core\Field\FieldItemBase::__unset() for everyone.

Couldn't we implement __unset() in EntityReferenceFieldItemList, EntityReferenceItemBase?

We could also do deprecation layer __set() for the typed data thing in the same two classes?

Status: Fixed » Closed (fixed)

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