Updated: Comment #N

Problem/Motivation

Hal uses setter injection from the NormalizerBase class. These are setLinkManager, and setEntityResolver. So some or all of these are being used for some of the services that extend this class. Also FieldNormalizer and FieldItemNormalizer do not need these but the link manager is being set on these too.

This makes it much more confusing to determine the dependencies for these services. These are also required dependencies, and don't need to be synchronized etc.. So setter injection doesn;t makes much sense.

Proposed resolution

Switch to constructor injection and pass the dependencies in as services instead.

Remaining tasks

Patch, reviews.

User interface changes

None

API changes

Removal of setLinkManager/setEntityResolver methods on hals NormalizerBase class. I would be very surprised if anyone (except maybe Lin) was extending any of these :)

CommentFileSizeAuthor
d8.hal-constructors.patch7.93 KBdamiankloip
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +D8MA, +SprintWeekend2014

This makes it much more confusing to determine the dependencies for these services. These are also required dependencies, and don't need to be synchronized etc.. So setter injection doesn;t makes much sense.

Especially the required bit is quite important. You want to make it as simple as possible.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Makes sense. Committed c3b1273 and pushed to 8.x. Thanks!

jessebeach’s picture

I'm not yet sure if it's related, but I just found this #2187413: GET request for an entity via application/hal+json throws a fatal error in service "current_user".

I'm going to attempt a bisect to see if I can find the commit that broke this behavior.

jessebeach’s picture

Status: Fixed » Closed (fixed)

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