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
- verify if still an issue
- update issue summary
- maybe reroll
Comment | File | Size | Author |
---|---|---|---|
#5 | 1907750-access-request-dependency-5.patch | 3.71 KB | scor |
#4 | 1907750-access-request-dependency.patch | 3.13 KB | Crell |
access_manager_request_dependency.patch | 910 bytes | effulgentsia | |
Comments
Comment #2
scor CreditAttribution: scor commentedthanks for debugging this @effulgentsia! tagging appropriately.
Comment #3
Crell CreditAttribution: Crell commentedI 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. :-)
Comment #4
Crell CreditAttribution: Crell commentedNice and simple.
Comment #5
scor CreditAttribution: scor commentedFollow 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.Comment #7
scor CreditAttribution: scor commented"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' ?
Comment #8
dcam CreditAttribution: dcam commentedhttp://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.
Comment #9
Berdir#5: 1907750-access-request-dependency-5.patch queued for re-testing.
Comment #11
BerdirCan someone check if this is still valid? I know we have separate issues about request and synthetic services, so maybe a duplicate?
Comment #12
Crell CreditAttribution: Crell commentedWe're already working on the synthetic service, so we need to solve that first: #2004086: The Request service must be synthetic
Comment #13
YesCT CreditAttribution: YesCT commentedthat is closed. re-opening this.
probably needs a reroll and/or someone to verify this is still a problem.
Comment #14
Sam Hermans CreditAttribution: Sam Hermans commentedHi, 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
Comment #15
WarrenK CreditAttribution: WarrenK commentedComment #16
WarrenK CreditAttribution: WarrenK commented