Updated: Comment #14

Problem/Motivation

ContainerBuilder throws an error upon trying to create an uninstantiated synthetic service.

Proposed resolution

Patch has been re-rolled as it is no longer of any use. All references to $this->request have been changed into Request $request.

Verified that this->request is not used in AccessManager.php.

Remaining tasks

None.

User interface changes

None.

API changes

None.

Original report by @effulgentsia

Spin-off from #1894002: Update vendor libraries and pin them to specific versions in composer.json.

That issue is trying to update to Symfony 2.2-BETA2. That Symfony update includes a fix to ContainerBuilder to throw an error upon trying to create an uninstantiated synthetic service. Throwing an error in that case is the correct action, and the pre-beta2 version of ContainerBuilder in Drupal HEAD is wrong to not be throwing that error.

That fix, however, exposes a Drupal bug, as demonstrated by the test failures caused by this patch.

The bug is that the 'access_manager' service depends on 'request'. However, during installation, drupal_flush_all_caches() is called which calls drupal_container()->get('router.builder')->rebuild(); which dispatches events listened to by the 'access_subscriber', which depends on 'access_manager'. However, since this is during installation, there is no 'request'.

Possible solutions:
1. Fix 'access_manager' to not have a hard dependency on 'request'. Its methods that get called during the router rebuild events don't need the request for anything. Only its other methods do.
2. Make the installer set a mock 'request' service instance in the DIC.

I think the first one would be cleaner. Feedback?

next steps

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch, access_manager_request_dependency.patch, failed testing.

scor’s picture

Issue tags: +WSCCI

thanks for debugging this @effulgentsia! tagging appropriately.

Crell’s picture

I agree. The access checker can take the request as a parameter at runtime, eliminating the dependency. That's probably my fault in the first place. :-)

Crell’s picture

Status: Needs work » Needs review
FileSize
3.13 KB

Nice and simple.

scor’s picture

Follow up patch after #1894002: Update vendor libraries and pin them to specific versions in composer.json was committed. This patch reverts the hunk which temporarily removed the setSynthetic(TRUE) on the request service.

Status: Needs review » Needs work

The last submitted patch, 1907750-access-request-dependency-5.patch, failed testing.

scor’s picture

"You have requested a synthetic service ("request"). The DIC does not know how to construct this service." that's the error I was getting at #1894002: Update vendor libraries and pin them to specific versions in composer.json before I removed the synthetic flag from the request service. Back then I was getting it straight up in the installer. Now it seems to install ok on the testbot and also on my machine (tested earlier before uploading this patch). So looks like there might be some other services depending on 'request' besides 'access_manager' ?

dcam’s picture

http://drupal.org/node/1427826 contains instructions for updating the issue summary with the summary template.

The summary may need to be updated with information from comments.

Berdir’s picture

Status: Needs work » Needs review
Issue tags: -Needs issue summary update, -WSCCI

Status: Needs review » Needs work
Issue tags: +Needs issue summary update, +WSCCI

The last submitted patch, 1907750-access-request-dependency-5.patch, failed testing.

Berdir’s picture

Can someone check if this is still valid? I know we have separate issues about request and synthetic services, so maybe a duplicate?

Crell’s picture

Status: Needs work » Postponed

We're already working on the synthetic service, so we need to solve that first: #2004086: The Request service must be synthetic

YesCT’s picture

Issue summary: View changes
Status: Postponed » Needs work
Issue tags: +Needs reroll

that is closed. re-opening this.
probably needs a reroll and/or someone to verify this is still a problem.

Sam Hermans’s picture

Status: Needs work » Closed (won't fix)
Issue tags: -Needs reroll

Hi, i re-rolled this patch, it is no longer of any use and all refrences to $this->request have allready been changed into Request $request.

See issue #2046737: Add a method to the AccessManager that only needs a route name and parameters

WarrenK’s picture

Issue summary: View changes
WarrenK’s picture

Issue summary: View changes