Problem/Motivation

In serialization.services.yml we have an empty array passed into the constructor for the ChainEntityResolver service. The RegisterEntityResolversCompilerPass then aggregates a list of resolvers and uses replaceArgument to add them to the ChainEntityResolver. This is not really a pattern we use in other places, and I think it is more confusing.

Proposed resolution

Replace this usage with addResolver calls on the serializer.entity_resolver service.

Remaining tasks

Do it

User interface changes

None

API changes

None

CommentFileSizeAuthor
#7 interdiff-2284413-7.txt1.84 KBdamiankloip
#7 2284413-7.patch5.98 KBdamiankloip
#3 2284413-3.patch4.93 KBdamiankloip
#1 2284413.patch5.18 KBdamiankloip

Comments

damiankloip’s picture

damiankloip commented
Status: Active » Needs review
StatusFileSize
new 2284413.patch5.18 KB
damiankloip’s picture

damiankloip commented

1: 2284413.patch queued for re-testing.

damiankloip’s picture

damiankloip commented
StatusFileSize
new 2284413-3.patch4.93 KB

Reroll after #697760: Replace getInfo() in tests with native phpDoc + annotations (following PHPUnit).

dawehner’s picture

dawehner
Primary language German
commented 
+++ b/core/modules/serialization/src/EntityResolver/ChainEntityResolver.php
@@ -32,6 +32,16 @@ public function __construct(array $resolvers = array()) {
+   */
+  public function addResolver(EntityResolverInterface $resolver) {

I wonder whether this means that we need a new interface here? I would say no

damiankloip’s picture

damiankloip commented

Yeah. I would agree no too I think :)

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

agreed

damiankloip’s picture

damiankloip commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new 2284413-7.patch5.98 KB
new interdiff-2284413-7.txt1.84 KB

Spoke to Alex, he would like to see an interface as the compiler pass depends on it.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

I especially think that we should be able to chain more resolvers later

alexpott’s picture

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

Committed a0290ec and pushed to 8.x. Thanks!

  • alexpott committed a0290ec on 8.0.x 
    Issue #2284413 by damiankloip: Use addMethodCall() instead of...

Status: Fixed » Closed (fixed)

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