Add event registration

[bomoko]

Overview

This pull request: Adds event dispatching for site aliases that are not found through the usual resolution methods

For the record, I'm so sorry this is the 3rd PR I've opened here - I didn't realize renaming the branch would close the last one. Massive apologies for the spam.

Q	A
Bug fix?	no
New feature?	yes
Has tests?	yes
BC breaks?	no
Deprecations?	no

Summary

Implemented the Symfony EventDispatcher component and introduced a new event

closes #66

[codecov]

Codecov Report

Merging #69 (2521bc8) into main (b0eeb8c) will increase coverage by 1.02%.
The diff coverage is 100.00%.

❗ Current head 2521bc8 differs from pull request most recent head f8f8b30. Consider uploading reports for the commit f8f8b30 to get more accurate results

@@             Coverage Diff              @@
##               main      #69      +/-   ##
============================================
+ Coverage     59.15%   60.17%   +1.02%     
- Complexity      283      290       +7     
============================================
  Files            12       13       +1     
  Lines           661      678      +17     
============================================
+ Hits            391      408      +17     
  Misses          270      270              

Files Changed	Coverage Δ
src/Events/AliasNotFoundEvent.php	100.00% <100.00%> (ø)
src/SiteAliasManager.php	62.31% <100.00%> (+5.65%)	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[greg-1-anderson]

Should $hook really be a parameter here? It seems that this should only allow setting our one type of event.

[bomoko]

For sure, I made it generic with the notion that there might be other event types in the future.
Should it still be called "addListener" then? Would a more specific name be more appropriate then?

[greg-1-anderson]

Naming things: I'd prefer to see this called something like AliasDiscoveryEvent, as AliasNotFoundEvent sounds like error reporting rather than a second-tier searching event.

[greg-1-anderson]

I'm skeptical about this addition. I understand the desire, but fallback handlers can be fraught. Drush 8 tried really hard to find aliases and do all of the things, which led to a really slow system. Drush 9+ has been a lot more opinionated about aliases. Only specific simple forms of alias records are supported, and the aliases all have to be present on the local disk (and can be found / not found with a single "file_exists" call). Allowing arbitrary discovery calls to be made would mean that any extension could add a handler that would make all alias discovery slower (or at least all error reporting for mistyped alias names).

I would be much more likely to want to merge a callback mechanism that only worked when the user specifically asked for it. e.g. because it only ran for aliases in one very specific namespace.

[greg-1-anderson]

See alias naming conventions. Perhaps when you register your callback, you provide a parameter that says that the directory must be "amazee"; then, if a user searches for @amazee.mysite.dev, and the file is not found, the discovery method is called. Maybe the discovery method returns only the alias record data, and the site-alias manager writes the data into the correct alias file, so the next call doesn't need to be made. Or maybe the callback handler is called even if the cached file is found, and the mod date of the file is included, so the handler can decide whether or not to do cache invalidation.

One more question: do you have a place where you know you can call SiteAliasManager::addListener() such that the listener is added early enough for Drush to use it? Alias resolution happens very early in the Drush bootstrap. If this PR is for use in some other application that isn't Drush, then this is not a concern.

[bomoko]

Hi @greg-1-anderson - Thanks for the feedback.

Let me try address your questions/issues before I respond to the code review with new code.

1 - I quite like the notion of namespace specific targeting. In our use case we've already had to namespace everything with @lagoon.X so we could certainly do that.

2 - I actually don't have a place where this could be integrated into Drush (it's indeed for Drush - but I've tried to make it as generic as I could) - I was actually going to follow up on your comment (drush-ops/drush#5273 (comment)) and ask for some guidance on this (since adding this to the SAM was initially your idea :) ).

[greg-1-anderson]

Yeah I didn't really remember that convo from a year ago. I'm not sure if my idea from there works. The listener would need to be added in Preflight::preflight() (or somewhere called from there), before the site alias manager is used to look up the alias from the command line. Registering commandfiles, on the other hand, happens really late, not until just before $application->run(). Even if we discovered commandfiles earlier, the earliest hook is not called until $application->run() takes over.

Maybe what we would need would be a new PSR4 discovery call similar to the existing one that looks for a different well-known relative namespace, and specifically adds everything found to the alias manager via the new addListener method provided here.

[bomoko]

Thanks again @greg-1-anderson - really appreciate your time on this.

We could definitely implement something like the PRS4 discovery call - if we nail what our addListener looks like, that should be straightforward.

I'm wondering, with your suggestion:
See [alias naming conventions](https://github.com/consolidation/site-alias#alias-naming-conventions). Perhaps when you register your callback, you provide a parameter that says that the directory must be "amazee"; then, if a user searches for @amazee.mysite.dev, and the file is not found, the discovery method is called.

I have the following idea. Perhaps the way to approach this is to still use the symfony component, but then when we call the "addListener" we provide a resolution scope that's tied to the event name.

So, in your example with the amazee directory - one would register something like
$sam->addListener("amazee", $myHook)

Which would then register an event named alias-discover-lagoon. What we could then do if we hit the point where the standard SAM operations fail to yield a siteAlias, we simply call the dispatcher and dispatch the event alias-discover-lagoon - this would circumvent having to trigger all the event listeners, and provide very targeted resolution.
I'll push some code that demos this.

[bomoko]

Oh, I wanted to add Maybe the discovery method returns only the alias record data, and the site-alias manager writes the data into the correct alias file, so the next call doesn't need to be made.

This is slightly challenging, I think, for some of the setups I see deploying containers - if we don't have alias files in persistent volumes, and lock down the filesystem, then this suddenly becomes a problem.
I totally understand why this makes sense - but I think having an entirely dynamic way of providing site alias data would still be massively useful.

[greg-1-anderson]

Not sure I understand what you're getting at with your comments on the symfony event dispatcher. My desire is that it shouldn't be possible to add a listener that is called for every failed resolution, but is only called for some alias namespaces. Any way that you achieve that is okay with me, as long as the listeners aren't given the opportunity to just ignore a parameter and thereby again listen to every failed resolution.

Regarding caching, it's fine with me to push that off to the listeners to do or not do, if it's challenging to do uniformly in the site alias manager. Maybe the alias manager could cache by default, and ignore failures? Or the listener result could have a "don't cache me" flag? Just musing, I do not feel strongly about the caching solution (or lack thereof).

[greg-1-anderson]

We should probably also check in with @weitzman and make sure that he doesn't mind adding preflight hooks to Drush.

[bomoko]

Not sure I understand what you're getting at with your comments on the symfony event dispatcher. My desire is that it shouldn't be possible to add a listener that is called for every failed resolution, but is only called for some alias namespaces. Any way that you achieve that is okay with me, as long as the listeners aren't given the opportunity to just ignore a parameter and thereby again listen to every failed resolution.

So sorry for the lack of clarity here - I'll shoot through some code ASAP, but I think we're on exactly the same page.

I'm suggesting something like changing the addListener to require a "location" parameter

so:
$sam->addListener("myfile", $hook);
And that then will scope the hook to only "myfile" events.

Let me send some code though, that'll be more concrete!

Thanks again for everything.