0
\$\begingroup\$

So I have the getSessionNamespace method in the below class in which I am trying to return different scenarios but one json response. So my solution is to store the required message and status code in a response array and vary it depending on the situation.

Is there a cleaner more obvious method?

<?php

namespace Drupal\auth\Controller;

use Drupal\Core\Controller\ControllerBase;
use Drupal\auth\Service\AuthGroupService;
use Symfony\Component\HttpFoundation\Cookie;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\DependencyInjection\ContainerInterface;
use Symfony\Component\HttpFoundation\JsonResponse;
use GuzzleHttp\Exception\ClientException;
use Symfony\Component\HttpKernel\Exception\HttpException;

/**
 * Class SessionController
 * @package Drupal\my_moduke\Controller
 */
class SessionController extends ControllerBase
{
    /**
     * @var AuthGroupService
     */
    protected static $authGroupService;

    /**
     * @param ContainerInterface $container
     *
     * @return static
     */
    public static function create(ContainerInterface $container)
    {
        self::$authGroupService = $container->get('auth.auth_group');
        return parent::create($container);
    }

    /**
     * @param Request $request
     * @param bool    $internal
     *
     * @return mixed|JsonResponse
     */
    public function getSessionNamespace(Request $request, $internal = false)
    {
        try {
            if (!$request->headers->has('host')) {
                throw new \HttpHeaderException('Header \'host\' not found.');
            }

            $host = $request->headers->get('host');

            // Query datastore for the authGroup data for the given host.
            $result = $this->getAuthGroupData($host);
            $configId = \Drupal::config('auth.settings')->get('DS5.auth_group_id');

            if (!$this->allowedAccess($configId, $result['authGroupId'])){
                throw new HttpException(403, 'Not allowed.');
            }

            $response = [
                'result' => $result,
                'status' => 200
            ];
        } catch (ClientException $e) {
            $response = [
                'result' => 'The endpoint responded with a ' . $e->getCode(),
                'status' => $e->getCode()
            ];
        } catch (\Exception $e) {
            $response = [
                'result' => $e->getMessage(),
                'status' => $e->getStatusCode()
            ];
        }

        if ($internal) {
            return $response['result'];
        }

        $jsonResponse = new JsonResponse($response['result'], $response['status']);

        if (!$_COOKIE[$response['result']['cookieName']] && $response['status'] === 200) {
            $this->createSessionCookie($jsonResponse, $response['result']);
        }

        return $jsonResponse;
    }

    /**
     * @param JsonResponse $response
     * @param array        $values
     *
     * @return JsonResponse
     */
    private function createSessionCookie(JsonResponse $response, array $values)
    {
        $dateTime = new \DateTime('+'. $values['sessionTimeToLiveSeconds'] .' seconds');

        $sessionCookie = new Cookie(
            $values['cookieName'],
            session_id(),
            $dateTime,
            '/',
            $values['domain']
        );

        $response->headers->setCookie($sessionCookie);
    }

    /**
     * @param $configId
     * @param $authGroupId
     *
     * @return bool
     */
    private function allowedAccess($configId, $authGroupId)
    {
        return $configId === $authGroupId;
    }

    /**
     * @param $host
     *
     * @return mixed
     */
    private function getAuthGroupData($host)
    {
        // Fetch from authGroup datastore endpoint.
        return self::$authGroupService->fetchAuthGroupData($host);
    }
}
\$\endgroup\$
4
\$\begingroup\$

I think this generally looks pretty good. I would consider two things:

  • First I don't like the "internal" option here which leads to mixed return values. Have you considered having getSessionNameSpaceInternal() which would call this method and extract the desired return value from the JsonResponse?
  • Doing the above would allow you to directly return from catch block s like this:

    } catch (ClientException $e) {
        return new JsonResponse(
            'The endpoint responded with a ' . $e->getCode(),
            $e->getCode()
        );
    } catch (\Exception $e) {
        return new JsonResponse( $e->getMessage(), $e->getStatusCode() );
    }
    
\$\endgroup\$
  • \$\begingroup\$ Yeah mixed returns are not great and it makes sense (cleaner) to split that out into a concisely names method. Thanks :) \$\endgroup\$ – Kal Feb 8 '17 at 11:04

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.