CRM-20445 Add dispatcher to modifyQuery

[eileenmcnaughton]

• CRM-20445: Add query dispatcher to allow query modification

This allows a query to be modified from an extension - e.g by prepending data like '/* Query run by user 45*/'

@totten this is working for me now....

[seamuslee001]

Jenkins test this pelase

[davejenx]

Hi @eileenmcnaughton, hope you're enjoying sunny Manchester!
Had a little look at this PR on the way back - just a few thoughts for now in lieu of actual testing...

1. Can a Symfony-style hook be used in a unit test, in order to test this PR?

2. Does it work if the hook injects a comment that has whitespace before the /* ? I think the check in DB::isManip() may not catch this, so the comment wouldn't be removed, so the function would return false. I haven't checked this or traced through to see what consequence that would have.

3. Does it allow through /* ... */ in quoted strings, e.g.

INSERT INTO civicrm_activity(..., details, ...)
VALUES (..., "Learnt that in MySQL, inline comments can be added /* like this */.", ...)
``` ?

[seamuslee001]

@eileenmcnaughton any thoughts on where to take this?

[eileenmcnaughton]

@seamuslee001 I guess the above is basically - 'needs test'

[eileenmcnaughton]

And finally civicrm/civicrm-core#14716

[eileenmcnaughton]

test this please

[davejenx]

Hi @eileenmcnaughton,

Have tested this successfully.

• General standards
  • Explain (r-explain)
    • SOFT PASS : The goal/problem/solution have been briefly explained with a link. An improvement would be to link to extension code that demonstrates its use and briefly describe the usage, e.g. the string added to the query must begin with /* with no leading whitespace, must be a valid MySQL comment and end with */. The added comment will appear in MySQL process list & query log but not in Civi query log.
  • User impact (r-user)
    • PASS: The change would be intuitive or unnoticeable for a majority of users who work with this feature.
  • Documentation (r-doc)
    • SOFT PASS: The changes do not strictly require documentation, as they will have no impact except for developers who wish to use the new hidden feature. But a brief addition to https://docs.civicrm.org/dev/en/latest/tools/debugging/ would be nice.
  • Run it (r-run)
    • PASS: after applying the patch, I installed org.wikimedia.userquerydata . Initially I tested be enabling CIVICRM_DEBUG_LOG_QUERY, however the added comment did not appear there. I then tested by repeatedly checking MySQL process list, where I did see the added comment. Also tested creating an activity with details text containing a MySQL comment, to make sure this wasn't broken by the change - all OK.
• Developer standards
  • Technical impact (r-tech)
    • PASS: I gather from comments that Tim was involved in formulating the approach, so if he's OK with it I'll go with... The change preserves compatibility with existing callers/code/downstream.
  • Code quality (r-code)
    • PASS: The functionality, purpose, and style of the code seems clear+sensible.
  • Maintainability (r-maint)
    • PASS: The change sufficiently improves test coverage, or the change is trivial enough that it does not require tests. - Test provided in Add unit test demonstrating attaching a listener to queries civicrm-core#14716 .
  • Test results (r-test)
    • UNREVIEWED
    • COMMENTS: haven't yet run the associated test.

[eileenmcnaughton]

Great @davejenx I'll merge this & re-run the test on master. I'll also comment the documentation issue on the test

[totten]

@eileenmcnaughton Regarding performance, the dispatch() should be pretty cheap on non-hook events.

Since this isn't actually a hook, and since we're likely to fire O(100)s of times, I'd lean pretty solidly against using GenericHookEvent for this. Instead, make a class that doesn't use any reflection or array-mangling, like:

namespace Civi\Core\Event;

/**
 * Class QueryEvent
 * @package Civi\Event
 */
class QueryEvent extends \Symfony\Component\EventDispatcher\Event {

  /**
   * @var string
   */
  public $query;

  /**
   * QueryEvent constructor.
   * @param string $query
   */
  public function __construct($query) {
    $this->query = $query;
  }

}

[eileenmcnaughton]

@totten lol - that was code you proposed in an earlier conversation - but OK - I think I see what you say we should change it to - let me try