Instead of injecting request service, rather use the Symfony 2.4 request_stack in AdminContext.

CommentFileSizeAuthor
#5 interdiff.txt861 bytesznerol
#5 2225539-use-request-stack-in-admin-context-5.diff2.62 KBznerol
#3 2225539-use-request-stack-in-admin-context-2.diff2.56 KBznerol
#1 2225539-use-request-stack-in-admin-context.diff2.16 KBznerol

Comments

znerol’s picture

znerol commented
Status: Active » Needs review
Issue tags: +Quick fix, +symfony
StatusFileSize
new 2225539-use-request-stack-in-admin-context.diff2.16 KB

Status: Needs review » Needs work

The last submitted patch, 1: 2225539-use-request-stack-in-admin-context.diff, failed testing.

znerol’s picture

znerol commented
Status: Needs work » Needs review
StatusFileSize
new 2225539-use-request-stack-in-admin-context-2.diff2.56 KB

Request stack may return NULL instead of a request object when accessed outside the request-response stack - e.g. when triggered from the simpletest parent site.

dawehner’s picture

dawehner
Primary language German
commented 
  1. +++ b/core/lib/Drupal/Core/Routing/AdminContext.php
@@ -17,20 +17,20 @@
-    $this->route = $request->attributes->get(RouteObjectInterface::ROUTE_OBJECT);

@@ -45,7 +45,7 @@ public function setRequest(Request $request) {
+      $route = $this->getRouteFromRequest();

    +1 to not get the information on constructor time. 

  2. +++ b/core/lib/Drupal/Core/Routing/AdminContext.php
@@ -53,4 +53,17 @@ public function isAdminRoute(Route $route = NULL) {
+  /**
+   * Extract the route object from the request if one is available.
+   */
+  protected function getRouteFromRequest() {

    Sadly this has a missing @return statement. 

  3. +++ b/core/lib/Drupal/Core/Routing/AdminContext.php
@@ -53,4 +53,17 @@ public function isAdminRoute(Route $route = NULL) {
+    $request = $this->requestStack->getCurrentRequest();
+    if ($request) {
+      $route = $request->attributes->get(RouteObjectInterface::ROUTE_OBJECT);
+      if ($route) {
+        return $route;
+      }

    I wonder whether we can get rid of one of the if statements.

znerol’s picture

znerol commented
StatusFileSize
new 2225539-use-request-stack-in-admin-context-5.diff2.62 KB
new interdiff.txt861 bytes

Thanks @dawehner for the review.

Sadly this has a missing @return statement.

Fixed

I wonder whether we can get rid of one of the if statements.

Yes, we can.

dawehner’s picture

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

Thank you

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Well that definitely looks much nicer!

Committed and pushed to 8.x. Thanks!

  • Commit 27a651a on 8.x by webchick: 
    Issue #2225539 by znerol: Use request stack in admin context service.

Status: Fixed » Closed (fixed)

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