Remove FormBuilder coupling to moduleHandler/themeManager

1. +++ b/core/lib/Drupal/Core/Form/FormAlterSubscriber.php
   @@ -0,0 +1,66 @@
   +    $events[FormEvents::ALTER][] = 'onFormAlter';
   

   <3 for proper constant usage!

2. +++ b/core/lib/Drupal/Core/Form/FormEvents.php
   @@ -0,0 +1,17 @@
   +final class FormEvents {
   +
   +  const ALTER = 'form_builder.alter';
   +
   +}
   

   I still don't get why everyone wants to mark them as final.

In reality I'd wait for #2328777: Refactor FAPI getCache()/setCache() into a standalone class but I know you'll handle reroll ;) So RTBC.

On the side note I was thinking what can I do with my own event subscriber. Can you come up with some ideas?

So overall, I can see the idea behind this. And I like that we retain BC so modules don't actually have to change any code. But we get into murky territory here by converting a one-off hook (and not just some silly bootstrap hook that 8 modules implement, but one of *the* most oft-used hooks, esp. in custom modules) to an event and not doing so across the board. Because of the frequency with which this hook is used, it also means that if this patch goes in, contrib is likely to result in a "potpourri" of different approaches, where module A uses hook_form_alter, and module B uses the FormAlterEvent and it generally gets much harder as a developer to skip back and forth between them. (I realize this isn't the intent, but I believe would be the effect, and if we're going to go that route, I'd much prefer to give all hooks this option via something like #1972304: Add a HookEvent).

Anyway, of the core committers, I feel like catch has the strongest opinions about when to / not to use the Symfony event system in lieu of / alongside the Drupal hook system, so assigning to him after talking to Tim.

It is not my role to agree or disagree with a D8 patch so as the D7 form maintainer: the ordering of hook_form_alter vis form_FORM_ID_alter is already a confusing mess.

I agree with @chx and @webchick but IMHO we are going to end up with event base solution anyway when we'll replace hooks with events. As @webchick said it is most used hook why not take this opportunity to encourage contrib developer to use events instead of hooks.

So +1 for event base solution.

What is event_dispatcher used for other than alter right now?

One advantage of the FormAlter object abstraction is that it decouples form altering from *any* particular implementation. That means there's no dependency on hooks, OR on EventDispatcher (hence my question above for what else it's used for). That allows us to vary the implementation later if needs be, or per-site. (So if someone really wanted events for alters as well on their site for some reason, that would be possible.)

chx suggested a similar approach a while back to eliminate the hook_query_alter() call in the database layer. I think it's a good idea but we never really got around to it. We should do the same there, too, though.

I wonder whether we considered to have a alterdecorator for forms instead of passing it along.

That would be possible, but would be somewhat more involved as we'd need to forward every method, not just the one we are throwing an alter on. I think in this case it's OK to make it an intrinsic part of the concept of form processing, just via a separate object.

This improves decoupling, has zero impact on module developers, and is a change only to a constructor signature. This very low impact, moderate benefit => IMO RTBCable.

If you swap out the new service, and for example don't invoke hook_form_alter() at all, then all existing form_alters become dead code. That doesn't look viable for any real use case. So while this patch removes an explicit dependency on the module and theme system, it does nothing to remove the implicit dependency which is very much baked in.

If the goal is to use the form builder entirely outside of Drupal that also doesn't seem feasible given the current state of form API. If someone can make the case that we're actually quite close to this point and this patch really helps there, then that might help a bit.

On top of that, the issue summary is completely contrary to the patch.

Needs reroll now.

I updated the IS with a better description of what the end result is. This doesn't allow us to use FAPI outside of core itself; it nominally moves us closer but the bigger point is just better decoupling and separation of concerns with FAPI. It's better quality code and a small benefit to testability, which IMO is justification enough as there's no downside to module developers. (In theory one could swap out the alter service but I really don't expect anyone to do so; that is in no way an argument that it shouldn't be its own class, however, as it's more about separation of concerns than making such swappability a user-facing "feature".)

Adding #2402445: Implement object oriented form alters as a related issue, since it also adds a FormAlterInterface, though an entirely different one, to solve a different problem.

rerolled

Back up to the committers. (Will retest momentarily.)

Oh yeah, this issue. :-)

I'll reroll.

It was actually still applying, though with big offsets, so here's a fresh one.

The test fail is "Updates for Contrib module one" in Drupal\locale\Tests\LocaleUpdateTest->testUpdateImportSourceRemote().
Seems unrelated?

And we're back.

Still back.

The test change replaces two mocks with one mock, I'm still missing how the additional indirection is worth introducing for having to mock one less thing. This feels like 'service-per-method' rather than real decoupling still, and there are other issues untangling actual dependencies we could be spending the energy on instead.

At @tim.plunkett's encouragement, I have taken a shot at turning hook_form_alter into an event. This patch removes FormAlterInterface in favor of FormAlterEvent, but it keeps the existing FormAlter class doing exactly what it was already doing -- it is merely trigged using the normal event dispatcher now.

I didn't update the tests (or comment anything, really), so it will probably definitely fail horribly. This is more a proof of concept at the moment, to test the waters and see if people want to pursue this approach further.

I apologize for the lack of an interdiff; some hunks of the original patch didn't apply against 8.3.x.

This oughta fix the tests.

#7, #9, #14, #24 and #47 on this issue (possibly others) haven't been responded to adequately yet to justify why we'd do this.