Merge pull request #111 from packbackbooks/mgmt-132-url-params

Clean up some URL parsing

src/Helpers/Helpers.php

@@ -8,4 +8,19 @@ public static function checkIfNullValue($value): bool
     {
         return !is_null($value);
     }
+
+    public static function buildUrlWithQueryParams(string $url, array $params = []): string
+    {
+        if (empty($params)) {
+            return $url;
+        }
+
+        if (parse_url($url, PHP_URL_QUERY)) {
+            $separator = '&';
+        } else {
+            $separator = '?';
+        }
+
+        return $url.$separator.http_build_query($params, '');
+    }
 }

src/LtiAssignmentsGradesService.php

@@ -33,12 +33,7 @@ public function putGrade(LtiGrade $grade, LtiLineitem $lineitem = null)
         $this->validateScopes([LtiConstants::AGS_SCOPE_SCORE]);
 
         $lineitem = $this->ensureLineItemExists($lineitem);
-
-        $scoreUrl = $lineitem->getId();
-
-        // Place '/scores' before url params
-        $pos = strpos($scoreUrl, '?');
-        $scoreUrl = $pos === false ? $scoreUrl.'/scores' : substr_replace($scoreUrl, '/scores', $pos, 0);
+        $scoreUrl = $this->appendLineItemPath($lineitem, '/scores');
 
         $request = new ServiceRequest(
             ServiceRequest::METHOD_POST,
@@ -115,11 +110,7 @@ public function findOrCreateLineitem(LtiLineitem $newLineItem): LtiLineitem
     public function getGrades(LtiLineitem $lineitem = null)
     {
         $lineitem = $this->ensureLineItemExists($lineitem);
-        $resultsUrl = $lineitem->getId();
-
-        // Place '/results' before url params
-        $pos = strpos($resultsUrl, '?');
-        $resultsUrl = $pos === false ? $resultsUrl.'/results' : substr_replace($resultsUrl, '/results', $pos, 0);
+        $resultsUrl = $this->appendLineItemPath($lineitem, '/results');
 
         $request = new ServiceRequest(
             ServiceRequest::METHOD_GET,
@@ -198,4 +189,18 @@ private function isMatchingLineitem(array $lineitem, LtiLineitem $newLineItem):
             $newLineItem->getResourceId() == ($lineitem['resourceId'] ?? null) &&
             $newLineItem->getResourceLinkId() == ($lineitem['resourceLinkId'] ?? null);
     }
+
+    private function appendLineItemPath(LtiLineitem $lineItem, string $suffix): string
+    {
+        $url = $lineitem->getId();
+        $pos = strpos($url, '?');
+
+        if ($pos === false) {
+            $url = $url.$suffix;
+        } else {
+            $url = substr_replace($url, $suffix, $pos, 0);
+        }
+
+        return $url;
+    }
 }

src/LtiNamesRolesProvisioningService.php

@@ -2,6 +2,8 @@
 
 namespace Packback\Lti1p3;
 
+use Packback\Lti1p3\Helpers\Helpers;
+
 class LtiNamesRolesProvisioningService extends LtiAbstractService
 {
     public const CONTENTTYPE_MEMBERSHIPCONTAINER = 'application/vnd.ims.lti-nrps.v2.membershipcontainer+json';
@@ -16,10 +18,8 @@ public function getScope(): array
      */
     public function getMembers(array $options = []): array
     {
-        $url = $this->getServiceData()['context_memberships_url'];
-        if (!empty($options)) {
-            $url .= '?'.http_build_query($options);
-        }
+        $url = Helpers::buildUrlWithQueryParams($this->getServiceData()['context_memberships_url'], $options);
+
         $request = new ServiceRequest(
             ServiceRequest::METHOD_GET,
             $url,

src/LtiOidcLogin.php

@@ -2,6 +2,7 @@
 
 namespace Packback\Lti1p3;
 
+use Packback\Lti1p3\Helpers\Helpers;
 use Packback\Lti1p3\Interfaces\ICache;
 use Packback\Lti1p3\Interfaces\ICookie;
 use Packback\Lti1p3\Interfaces\IDatabase;
@@ -41,17 +42,17 @@ public static function new(IDatabase $database, ICache $cache = null, ICookie $c
     /**
      * Calculate the redirect location to return to based on an OIDC third party initiated login request.
      *
-     * @param  string  $launch_url URL to redirect back to after the OIDC login. This URL must match exactly a URL white listed in the platform.
-     * @param  array|string  $request    An array of request parameters. If not set will default to $_REQUEST.
+     * @param  string  $launchUrl URL to redirect back to after the OIDC login. This URL must match exactly a URL white listed in the platform.
+     * @param  array  $request    An array of request parameters. If not set will default to $_REQUEST.
      * @return Redirect returns a redirect object containing the fully formed OIDC login URL
      */
-    public function doOidcLoginRedirect($launch_url, array $request = null)
+    public function doOidcLoginRedirect($launchUrl, array $request = null)
     {
         if ($request === null) {
             $request = $_REQUEST;
         }
 
-        if (empty($launch_url)) {
+        if (empty($launchUrl)) {
             throw new OidcException(static::ERROR_MSG_LAUNCH_URL, 1);
         }
 
@@ -72,13 +73,13 @@ public function doOidcLoginRedirect($launch_url, array $request = null)
         $this->cache->cacheNonce($nonce, $state);
 
         // Build Response.
-        $auth_params = [
+        $authParams = [
             'scope' => 'openid', // OIDC Scope.
             'response_type' => 'id_token', // OIDC response is always an id token.
             'response_mode' => 'form_post', // OIDC response is always a form post.
             'prompt' => 'none', // Don't prompt user on redirect.
             'client_id' => $registration->getClientId(), // Registered client id.
-            'redirect_uri' => $launch_url, // URL to return to after login.
+            'redirect_uri' => $launchUrl, // URL to return to after login.
             'state' => $state, // State to identify browser session.
             'nonce' => $nonce, // Prevent replay attacks.
             'login_hint' => $request['login_hint'], // Login hint to identify platform session.
@@ -87,18 +88,13 @@ public function doOidcLoginRedirect($launch_url, array $request = null)
         // Pass back LTI message hint if we have it.
         if (isset($request['lti_message_hint'])) {
             // LTI message hint to identify LTI context within the platform.
-            $auth_params['lti_message_hint'] = $request['lti_message_hint'];
+            $authParams['lti_message_hint'] = $request['lti_message_hint'];
         }
 
-        if (parse_url($registration->getAuthLoginUrl(), PHP_URL_QUERY)) {
-            $separator = '&';
-        } else {
-            $separator = '?';
-        }
-        $auth_login_return_url = $registration->getAuthLoginUrl().$separator.http_build_query($auth_params, '', '&');
+        $authLoginReturnUrl = Helpers::buildUrlWithQueryParams($registration->getAuthLoginUrl(), $authParams);
 
         // Return auth redirect.
-        return new Redirect($auth_login_return_url, http_build_query($request, '', '&'));
+        return new Redirect($authLoginReturnUrl);
     }
 
     public function validateOidcLogin($request)

tests/Helpers/HelpersTest.php

@@ -0,0 +1,33 @@
+<?php
+
+namespace Tests\Helpers;
+
+use Packback\Lti1p3\Helpers\Helpers;
+use Tests\TestCase;
+
+class HelpersTest extends TestCase
+{
+    public function testItBuildsAUrlWithNoParams()
+    {
+        $expected = 'https://www.example.com';
+        $actual = Helpers::buildUrlWithQueryParams($expected);
+
+        $this->assertEquals($expected, $actual);
+    }
+
+    public function testItBuildsAUrlWithParams()
+    {
+        $baseUrl = 'https://www.example.com';
+        $actual = Helpers::buildUrlWithQueryParams($baseUrl, ['foo' => 'bar']);
+
+        $this->assertEquals('https://www.example.com?foo=bar', $actual);
+    }
+
+    public function testItBuildsAUrlWithExistingParams()
+    {
+        $baseUrl = 'https://www.example.com?baz=bat';
+        $actual = Helpers::buildUrlWithQueryParams($baseUrl, ['foo' => 'bar']);
+
+        $this->assertEquals('https://www.example.com?baz=bat&foo=bar', $actual);
+    }
+}