CRM-21659 - Add hook to CRM_Utils_System::redirect #11519

eileenmcnaughton · 2018-01-15T05:39:40Z

Overview

Adds a hook when the browser is being redirected. This allows extensions to override the destination of an HTTP redirect.

Before

It is not possible to alter redirects from the code
There is no formal model for manipulating URLs.

After

Extensions can manipulate redirects using hook_civicrm_alterRedirect(\Psr\Http\Message\UriInterface $url, array &$context)
The UriInterface from PSR-7 is included; the concrete implementation of Uri is loaded from Guzzle v6.3+.

Technical Details

Here is an example of changing the query string in the redirect URL:

function myredir_civicrm_alterRedirect(&$uri, &$context) {
  // You might replace all the query-parameters with new ones...
  $uri = $uri->withQuery('foo=bar');

  // Or you might prepend a query parameter...
  $uri = $uri->withQuery('whiz=bang&' . $uri->getQuery());
}

Comments

Will do PR for documentation once approved

CRM-21659: Add hook to CRM_Utils_System::redirect

adevapp · 2018-01-15T05:48:33Z

Good idea.

What is the best way to define execution context? - Is it assumed the developer will use other hooks and their variables ($form, $id, etc) to control when this hook is executed?

nganivet · 2018-01-15T05:48:34Z

@eileenmcnaughton What are some use case for this new hook? Why do we need it?

eileenmcnaughton · 2018-01-15T06:01:43Z

The specific use case is on the JIRA - ie to alter the parameters in a url that a redirect is going to after deduping. Obviously we do redirects in other places - e.g after contribution pages - depending on how the page is written it's possible to do the redirect with existing hooks or not

eileenmcnaughton · 2018-01-15T06:04:49Z

@adam-devapp I was looking at calling from an existing function that has very little info - but it might make more sense to wrap it in a more context-aware function

eileenmcnaughton · 2018-01-15T06:20:23Z

nb test fail is something happening on the server of late - unrelated

adixon · 2018-01-15T16:04:32Z

Good idea, code looks excellent. Two things:

Drupal's equivalent is hook_drupal_goto_alter, and the only context provided is the url, options (things like query strings, absolute vs. internal paths, etc.), and the http response code. Conclusion: @adam-devapp I suspect that any implementation of the hook would use pretty unpredictable context information, so trying to anticipate it in the calling arguments wouldn't be worthwhile, you could just pull it in via globals if you need it.
It would be great for a debug module to be able to implement a hook like this.

totten · 2018-01-15T18:40:00Z

CRM/Utils/Hook.php

+   * @return mixed
+   */
+  public static function alterRedirect($url) {
+    return self::singleton()->invoke(1, $url,


Use the notation `invoke(array('url'), $url, ...) to fully support all styles of listeners (e.g. Drupal hooks and Symfony events).

OK I made this change

totten · 2018-01-15T18:42:23Z

CRM/Utils/Hook.php

+   *
+   * @return mixed
+   */
+  public static function alterRedirect($url) {


Shouldn't the parameter be alterable? To my reading, a listener who prefers to alter the URL would need to execute the redirect themselves (e.g. emitting the Location: header and exit()ing).

If you read the code in CRM_Utils_System::redirect(), there's actually a bit nuance to how the redirect is executed. (Ex: An AJAX snippet works different from a standalone page. Ex: it's not exit(); it's civiExit().) The hook consumer should be focused on the content of the URL -- not on the mechanism of delivering that content.

I changed it to &$url

totten · 2018-01-15T18:45:51Z

CRM/Utils/Hook.php

+   * This hook is called when the browser is being re-directed and allows the url
+   * to be altered.
+   *
+   * @param string $url


Not a hard blocker, but just to verbalize -- wouldn't it scale-up better (to support many different listeners) to pass the URL in array or object format (rather than string format)? i.e. it would avoid redundant parsing/encoding logic?

I'm not 100% sure what you mean here - the url is a string in the redirect function - are suggesting unpacking it in that function to pass out as an array & recompiling it afterwards?

Maybe replace line 443 in CRM/Utils/System.php with a more sophisticated conversion of the url into an object (e.g. $parsed_url = parse_url($url)), and then pass that into alterRedirect. Then any alter functions don't have to reparse it, since I would assume the first order of any alter would be to do things like see if it's sending you offsite, or going to a particular path. Also, not clear of the value of returning something from the alterRedirect function since the url is going to be modified by reference, and the caller on 445 isn't paying attention to the return value.

@adixon Yeah, parse_url() should do a pretty good job. Might also add parse_str() so that the query-parameters are an array:

$parsedUrl = parse_url($url); if (isset($parsedUrl['query'])) { parse_str($parsedUrl['query'], $parsedUrl['query']); }

(Example inputs/outputs: https://gist.github.com/totten/82918724be1293ffed4e4f7f87d01706 )

It takes a bit more code to put it back into string form. Here's an example of re-encoding the URL.

Re: return -- I think that's just pro-forma. Most hook-stubs have the return statement, and the IDE really wants you want to document it, so you see a lot of snippets like this:

* @return null * the return value is ignored

So to be clear @totten - you agree with @adixon that it would be better to call parse url & then re-encode the string after the hook?

I'm happy to do that - I just want to be sure there is consensus on it

@eileenmcnaughton Yup.

At first I was skeptical of parsing and re-encoding all redirected URLs all of the time, but realistically any consumer of this hook is going to be doing just that in order to decide whether to alter the redirect.

I've updated to include the parse + unparse & to have some unit tests on that. I did add the $context variable as it helped with the tests & I felt it would be easier to add now than to decide to later

eileenmcnaughton · 2018-01-15T19:37:52Z

@adixon @adam-devapp - does it make sense to add a second parameter $context = array()n then we could add the context in the calling function if desired (although this could equally be done later when such a desire arose)

eileenmcnaughton · 2018-01-15T20:54:50Z

test this please

adevapp · 2018-01-15T21:48:16Z

Hi Eileen, yes I agree - an array can be useless, as the values can change and may not even exist.

It's a pity you can't pass a request object so you can get a range of parameters and methods with predictable method outcomes - Similar to the request object in laravel i..e https://laravel.com/docs/5.5/requests

i.e. $url = $request->setUrl($newUrl);

eileenmcnaughton · 2018-01-15T23:30:14Z

test this please

agh1 · 2018-01-16T17:00:06Z

I like this in general. I like the idea of passing some context to CRM_Utils_System::redirect() that can go through to the hook.

I also think it would be helpful to pull the condition CRM_Utils_Array::value('snippet', $_GET) === 'json' into a variable that can be passed to the hook and potentially modified there. That would allow extension developers to handle things differently depending upon that context.

eileenmcnaughton · 2018-01-22T01:49:32Z

I added snippet parsing too per #11519 (comment)

totten · 2018-01-30T00:35:56Z

@eileenmcnaughton I like that the parsing/unparsing have been split out into separate functions that mirror each other. And that there are tests. 👍

FWIW, @adam-devapp, I had a similar thought. It would be better to use a request object, but that is a bigger task (since we don't really have a request object anywhere right now).

Tangential: PSR-7 defines a UriInterface which is rather on-point.

I think this is mergeable (i.e. consistent with other code/standard). However, if time permitted, I'd make one of these two changes:

Move the parseUrl()/unparseUrl() helpers from CRM/Utils/System.php to CRM/Utils/URL.php. These functions seem fairly small and loosely-coupled. Try to keep CRM_Utils_System from getting bigger.
Define the hook using UriInterface instead of array(). Write an implementation, or grab one from packagist. ( https://github.com/byjg/uri seems fine in skimming.)

totten · 2018-02-02T02:18:17Z

The test-failure seems to involve the new Guzzle dependency.

Wait a minu-- Guzzle! That's a bigger deal than byjg/uri. ;)

(a) I would love to have a proper HTTP client. Guzzle seems to be the most popular...

(b) In the past, I've given folks a hard time about using Guzzle in core, but now that PHP 5.3 is out of the picture, maybe my main argument is gone. There's still some concern that it's too popular (e.g. prone to conflict on custom builds)... but I'm tired of being the buzzkill on Guzzle, and the one concrete case (D8) might work anyway (since D8 is moving toward composer-based distribution, which means better dependency-matching). I'm inclined to toss this to a bigger list and see if anyone actually has a complaint about adding it...

CC @jackrabbithanna @dsnopek @artfulrobot

eileenmcnaughton · 2018-02-02T04:11:15Z

Side note - the reason for the Guzzle fail is that it works on the version of Guzzle that works with php 5.5+ (& which has the PSR interface) - when I adjusted to php 5.4 it got an earlier version of Guzzle. I think we should aim to resolve this after Wed next week against 5.5

eileenmcnaughton · 2018-02-02T04:46:22Z

(I just switched back to 5.5 version - I expect it will fail differently now :-)

artfulrobot · 2018-02-02T09:17:17Z

FWIW, I have brought guzzle into some (one?) extension. From memory I can't recall whether this was me deliberately adding it or whether it was brought in as a sub-dependency by a third party dependency (e.g. GoCardless Pro API uses itt). Hmmm, can't see this ending nicely for a while!

JoeMurray · 2018-02-02T13:17:57Z

Thanks everyone.
I'd just like to say that at a high level I really like a few things going on here: 1) concern for better coding style/practice re: keeping CRM_Utils_System from bloating, 2) moving towards standard approaches and libraries like PSR-7 and especially guzzle when we first bring in a new pattern to the codebase. Too often we only post criticisms. Today: Good work everyone!

davialexandre · 2018-02-12T13:38:31Z

CRM/Utils/Hook.php

+   * @return null
+   *   the return value is ignored
+   */
+  public static function alterRedirect(&$url, &$outputFormat, $context) {


Since $url is now an object, the & operator isn't necessary anymore

Good point - I've done that. I'm a bit torn though now as technically I should re-order & pass $outputFormat first since it's still a reference - but it is also the 'barely worth including' param in the set - I only passed it since @agh1 seemed to see some possible use case

Is there a rule that says that references should come before the other params? Does the order make any difference in that case?

IMHO, there's no obligation to put references first. Rather, just put the most important/salient field first.

In the case of alter-style hooks, the most important/salient field is also often a reference, so it may often look like references come first. But that's happenstance (to my eye).

davialexandre · 2018-02-12T13:42:04Z

CRM/Utils/Url.php

+   *
+   * @return string
+   */
+  static public function unparseUrl(UriInterface $parsed) {


Maybe this should be public static, as the parseUrl() method?

thanks -changed

davialexandre · 2018-02-12T13:44:02Z

tests/phpunit/CRM/Utils/UrlTest.php

@@ -0,0 +1,84 @@
+<?php
+
+use Psr\Http\Message\UriInterface;


This test looks exactly the same as the one you've added to the CRM_Utils_System class. Maybe here it would make more sense to test the parseUrl() and unparseUrl() method?

I've moved it out of system - it covers parse & unparse implicitly

davialexandre · 2018-02-12T13:45:01Z

tests/phpunit/CRM/Utils/UrlTest.php

+ * Class CRM_Utils_UrlTest
+ * @group headless
+ */
+class CRM_Utils_URLTest extends CiviUnitTestCase {


Given the class being tested is named CRM_Utils_Url shouldn't the test be CRM_Utils_UrlTest instead of CRM_Utils_URLTest?

opps - it should have been removed when I moved it to URLTest after moving the functions - gone now

I'm not sure I'm following. I was talking about the class names. The class being tested is named CRM_Utils_Url, but the test class is CRM_Utils_URLTest (while the file is UrlTest.php). Basically, you're using all uppercase letters for the URL word in the test class name and Url for all the rest

ah - good point!

davialexandre · 2018-02-14T18:59:06Z

tests/phpunit/CRM/Utils/UrlTest.php

+   *
+   * @dataProvider getURLs
+   */
+  public function testRedirectHook($url, $parsedUrl) {


I'm not sure if it makes sense for this test to be here. The CRM_Utils_Url class doesn't know anything about hooks or redirects. It only parses and unparses URLs. That's why I've mentioned here this I think we should use this test class to only test the CRM_Utils_Url methods and do the redirect/hook tests in the CRM_Utils_SystemTest class

hmm - well it DOES test the parse / unparse in that test. I can move it back but I think the test itself gives adequate cover of those

OK - I've moved it back into the System test & ditched the UrlTest - I think the parsing / unparsing is covered enough by the test but it does make more sense to be in the headline tested class

eileenmcnaughton · 2018-02-19T21:24:20Z

@totten I looked more closely & the actual interface being used here is from "psr/http-message" - version 1 is required by both guzzle & httplug with one using ~ & one ^ which is effectively the same thing at the moment. So, in order to get this merged I backed up to adding psr/http-message to composer.json & using that. I still think the discussion supported adding guzzle to our vendor for 5.x / april release but will move to a separate PR.

I think this is ready to 'merge on pass' having had a few rounds of review & being more or less approved prior to adding the whole psr-7 thing

eileenmcnaughton · 2018-02-19T21:34:44Z

I take it back - it IS using the guzzle bit - I'll put guzzle back. I think we can merge this without closing the httplug question

totten · 2018-03-14T01:24:31Z

composer.json

@@ -54,7 +54,8 @@
    "pear/Auth_SASL": "1.1.0",
    "pear/Net_SMTP": "1.6.*",
    "pear/Net_socket": "1.0.*",
-    "civicrm/civicrm-setup": "~0.2.0"
+    "civicrm/civicrm-setup": "~0.2.0",
+    "guzzlehttp/guzzle": "^6.3"


I wanted to do a quick sanity check to ensure that we wouldn't be throwing a wrinkle in D8 support. This looks OK -- one should be able to satisfy this constraint as well as all these variants from D8:

Drupal 8.0.x: "guzzlehttp/guzzle": "~6.1",

Drupal 8.1.x: "guzzlehttp/guzzle": "^6.2.1",

...

Drupal 8.6.x: "guzzlehttp/guzzle": "^6.2.1",

totten · 2018-03-14T01:35:20Z

CRM/Utils/System.php

+    $output = CRM_Utils_Array::value('snippet', $_GET);
+
+    $parsedUrl = CRM_Utils_Url::parseUrl($url);
+    CRM_Utils_Hook::alterRedirect($parsedUrl, $context, $output);


@eileenmcnaughton , this doesn't seem match-up with the function signature:

public static function alterRedirect($url, &$outputFormat, $context) {

Which order seems more correct?

What's an example where the outputFormat matters or needs to be altered?

Could we treat the outputFormat as part of $context?

I think output format could be part of context

OK @totten have fixed to be one array

totten · 2018-03-14T01:40:25Z

tests/phpunit/CRM/Utils/SystemTest.php

+   * @dataProvider getURLs
+   */
+  public function testRedirectHook($url, $parsedUrl) {
+    $this->hookClass->setHook('civicrm_alterRedirect', array($this, 'hook_checkUrl'));


This confused me a little while reading -- in the vast majority of test classes, a function named hook_foo is an implementation of hook_foo. Here the names don't match up. Suggestion: https://gist.github.com/totten/b5570ae6062fc115c4fb5e300385543b

Other examples:

tests/phpunit/api/v3/ContributionPageTest.php: public function hook_civicrm_alterPaymentProcessorParams($paymentObj, &$rawParams, &$cookedParams) { tests/phpunit/api/v3/MembershipTest.php: public function hook_civicrm_pre_update_create_membership($op, $objectName, $id, &$params) { tests/phpunit/api/v3/SelectQueryTest.php: public function hook_civicrm_selectWhereClause($entity, &$clauses) { tests/phpunit/Civi/Angular/ManagerTest.php: public function hook_civicrm_alterAngular($angular) { tests/phpunit/Civi/Angular/ManagerTest.php: public function hook_civicrm_angularModules_fooBar(&$angularModules) { tests/phpunit/Civi/CCase/SequenceListenerTest.php: public function hook_caseTypes(&$caseTypes) { tests/phpunit/Civi/Core/Event/GenericHookEventTest.php: public function hook_civicrm_ghet(&$roString, &$rwString, &$roArray, &$rwArray, $plainObj, &$refObj) { tests/phpunit/Civi/Test/ExampleHookTest.php: public function hook_civicrm_alterContent(&$content, $context, $tplName, &$object) { tests/phpunit/CiviTest/CiviCaseTestCase.php: public function hook_caseTypes(&$caseTypes) { tests/phpunit/CRM/ACL/ListTest.php: public function hook_civicrm_aclWhereClause($type, &$tables, &$whereTables, &$contactID, &$where) { tests/phpunit/CRM/Case/BAO/CaseTypeForkTest.php: public function hook_caseTypes(&$caseTypes) { tests/phpunit/CRM/Contribute/Form/Task/PDFLetterCommonTest.php: public function hook_tokens(&$tokens) { tests/phpunit/CRM/Contribute/Form/Task/PDFLetterCommonTest.php: public function hook_aggregateTokenValues(&$values, $contactIDs, $job = NULL, $tokens = array(), $context = NULL) { tests/phpunit/CRM/Core/FieldOptionsTest.php: public function hook_civicrm_fieldOptions($entity, $field, &$options, $params) { tests/phpunit/CRM/Group/Page/AjaxTest.php: public function hook_civicrm_aclGroup($type, $contactID, $tableName, &$allGroups, &$currentGroups) { tests/phpunit/CRM/Mailing/MailingSystemTest.php: public function hook_alterMailParams(&$params, $context = NULL) { tests/phpunit/CRM/Utils/SystemTest.php: public function hook_civicrm_alterRedirect($urlQuery, $context) {

fixup CRM-21659 Add hook to CRM_Utils_System::redirect This renames the hook function to be stylistically more consistent. In other tests, the function name matches the hook name.

totten · 2018-03-14T22:33:54Z

Kudos @eileenmcnaughton for patience with all rounds on this one. Final review:

(CiviCRM Review Template WORD-1.1)

(r-explain) Pass: I updated the PR description a little more.
(r-test) Pass
(r-code) Pass
(r-doc) Provisional Pass: Eileen said she'd update civicrm-dev-docs after merging the main PR. Don't need to hold it up.
(r-maint) Pass: New test-coverage. Hoowa!
(r-run) WIP: I'm doing this right now. Will update+merge afterward.
(r-user) Pass: Not user visible
(r-tech) Pass: On existing systems/integrations, this is a null-op.

PSR-7 specifies that the `UriInterface` is immutable. There are methods like `withQuery(...)` which generate a *new instance*. For the hook to support altering the URL, you must be able to replace the `$url` with a newer instance.

totten · 2018-03-15T05:25:41Z

While r-running the latest revision, I found that it wasn't working to revise the URL. Pushed up a commit with fix+explanation. Adding an example snippet of how to use the hook in the description.

If you're OK with this change, then I'd say it's merge-on-pass.

eileenmcnaughton · 2018-03-15T05:38:35Z

OK - that makes sense although I might need to re-read to fully take that complexity in - I'd only just embraced the fact you don't need to pass objects by ref :-)

civicrm-builder added the master label Jan 15, 2018

eileenmcnaughton force-pushed the hook branch from dd182cf to 5633848 Compare January 15, 2018 05:43

totten reviewed Jan 15, 2018

View reviewed changes

eileenmcnaughton force-pushed the hook branch from 5633848 to ac1cf90 Compare January 15, 2018 19:54

eileenmcnaughton force-pushed the hook branch 2 times, most recently from d7348a0 to cd4d48c Compare January 22, 2018 01:49

eileenmcnaughton force-pushed the hook branch 2 times, most recently from 87d83e7 to c840ce7 Compare January 22, 2018 21:02

eileenmcnaughton force-pushed the hook branch 3 times, most recently from d76b02e to 3abe476 Compare February 1, 2018 07:42

eileenmcnaughton force-pushed the hook branch from 3abe476 to 41c00d4 Compare February 2, 2018 04:45

eileenmcnaughton force-pushed the hook branch from 41c00d4 to faf8c91 Compare February 2, 2018 21:52

davialexandre reviewed Feb 12, 2018

View reviewed changes

eileenmcnaughton force-pushed the hook branch from faf8c91 to 8ae8064 Compare February 12, 2018 17:37

davialexandre reviewed Feb 14, 2018

View reviewed changes

eileenmcnaughton force-pushed the hook branch 3 times, most recently from 431dff1 to 50b621e Compare February 19, 2018 21:21

eileenmcnaughton force-pushed the hook branch from 50b621e to a56b2d1 Compare February 19, 2018 21:51

eileenmcnaughton mentioned this pull request Feb 20, 2018

Update to PHPWord 0.14.0 #11696

Merged

eileenmcnaughton force-pushed the hook branch from a56b2d1 to 2705a97 Compare February 27, 2018 01:58

totten reviewed Mar 14, 2018

View reviewed changes

eileenmcnaughton force-pushed the hook branch 2 times, most recently from 529485d to eb4c6f6 Compare March 14, 2018 02:04

CRM-21659 Add hook to CRM_Utils_System::redirect

f9bdf06

fixup CRM-21659 Add hook to CRM_Utils_System::redirect This renames the hook function to be stylistically more consistent. In other tests, the function name matches the hook name.

eileenmcnaughton force-pushed the hook branch from eb4c6f6 to f9bdf06 Compare March 14, 2018 02:23

totten changed the title ~~CRM-21659 Add hook to CRM_Utils_System::redirect~~ CRM-21659 - Add hook to CRM_Utils_System::redirect Mar 14, 2018

CRM_Utils_Hook::alterRedirect - Pass URL by reference

ca4ce86

PSR-7 specifies that the `UriInterface` is immutable. There are methods like `withQuery(...)` which generate a *new instance*. For the hook to support altering the URL, you must be able to replace the `$url` with a newer instance.

eileenmcnaughton added the merge on pass label Mar 15, 2018

eileenmcnaughton merged commit 8c6a526 into civicrm:master Mar 15, 2018

eileenmcnaughton deleted the hook branch March 15, 2018 07:48

CRM-21659 - Add hook to CRM_Utils_System::redirect #11519

CRM-21659 - Add hook to CRM_Utils_System::redirect #11519

Conversation

eileenmcnaughton commented Jan 15, 2018 • edited by totten Loading

Overview

Before

After

Technical Details

Comments

adevapp commented Jan 15, 2018

nganivet commented Jan 15, 2018

eileenmcnaughton commented Jan 15, 2018

eileenmcnaughton commented Jan 15, 2018

eileenmcnaughton commented Jan 15, 2018

adixon commented Jan 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eileenmcnaughton commented Jan 15, 2018

eileenmcnaughton commented Jan 15, 2018

adevapp commented Jan 15, 2018

eileenmcnaughton commented Jan 15, 2018

agh1 commented Jan 16, 2018

eileenmcnaughton commented Jan 22, 2018

totten commented Jan 30, 2018

totten commented Feb 2, 2018

eileenmcnaughton commented Feb 2, 2018

eileenmcnaughton commented Feb 2, 2018

artfulrobot commented Feb 2, 2018

JoeMurray commented Feb 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eileenmcnaughton commented Feb 19, 2018

eileenmcnaughton commented Feb 19, 2018

totten Mar 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

totten commented Mar 14, 2018

totten commented Mar 15, 2018

eileenmcnaughton commented Mar 15, 2018

eileenmcnaughton commented Jan 15, 2018 •

edited by totten

Loading

totten Mar 14, 2018 •

edited

Loading