Core itself is (as far as I’m aware) still very ambivalent about whether to prefer hooks or events, but it seems to me like events are the better option all around: more standard (vs. the hook Druaplism), auto-loaded, self-contained, object-oriented. Only documentation might be better for hooks, I’m not aware of any centralized way of documenting all the events a module provides. (But maybe this could be achieved by having all events in the \Drupal\search_api\Event
namespace?)
Anyways, as long as we also keep the hooks for now (but trigger an E_USER_DEPRECATED
error), we are free to start this process at any time, without breaking BC. The hooks could then be removed completely as a second step in an 8.x-2.x (or 9.x-1.x) branch.
One potential problem I see is that, as long as Core doesn’t move towards events, too, we’ll still be exposing hooks (or at least should, for the sake of consistency) for things like altering plugin definitions. So, a complete move to events doesn’t appear (easily) possible at this moment. Same for the entity CRUD events (though #2551893: Add events for matching entity hooks would fix that).
Further reading:
- Blog post by Lee Rowlands
- #2264049: Create an Events topic
- #1751116: [policy, then patch] Determine standard for documenting Events and Subscribers/Listeners
Specific issues for moving certain hooks to events (e.g., #2249377: Convert views hooks to events) in Core seem to have become stuck. But tehy, too, point to events being the preferred option.
(Side note: I’m pretty sure we already discussed this at some point during the initial Drupal 8 port, but unfortunately I can’t find any such discussion anymore.)
Comment | File | Size | Author |
---|---|---|---|
#33 | 3023704-33--convert_hooks_to_events.patch | 93.21 KB | drunken monkey |
|
Comments
Comment #2
drunken monkeyNote: There’s also the Hook Event Dispatcher module, which allows people to use events in place of hooks on the implementer-side of things. But not really relevant for us, I think.
Comment #3
legolasboI'm taking a stab at this and have attached a first WIP patch. Planning to continue work on this tomorrow.
Comment #4
legolasboAdded a bunch more events let's see what testbot thinks about it before I continue working on this later.
Comment #5
legolasboI've added a couple extra events, but chose to skip
hook_search_api_index_items_alter
because that's already deprecated. I also noticed that hook_query_TAG_alter was never invoked (the test didn't test for any measurable effect), which is also fixed in this patch.Only 5 more events to go!
Comment #6
legolasboThere, that's all of them! :)
Comment #7
legolasboAnd that should fix the last coding standards issues.
Comment #8
legolasboThis should really fix the last one.
Comment #9
borisson_Are we doing this because objects are passed by reference by default or there another reason for doing this? In any case, I'm not sure this should be in this patch. I guess it could be a seperate patch as well?
Not sure, @drunken monkey what do you think?
Nice! I like this notation a lot more as well.
This should probably be lowercase
class
.This is twice in the same method that we're getting the event dispatcher. We are also getting the event dispatcher a few other times in the same class.
Do you think it'd be better for readability if we move this a protected method? It would only save us one line of code and one line of docs per instance (8 lines in total). So not really I guess.
@drunken monkey do you have an opinion here?
/s/private/protected/
It seems like you made most of the properties of the events private. I think we want to have them protected like we do with most (all) of our other properties.
This is for all of the implementations of this method. Should we add something like:
Deprecated in search_api:8.14, will be removed in the next major release. Use %s instead.
We can't really say what the next major will be. It will either be a 9.1-x or a 8.2-x (probably a 9 version tbh) but adding that information in the message might be too much.
Comment #10
legolasboAnyway, improved version below :)
Comment #11
legolasboComment #12
legolasbo#3055775: Remove explicit references to implicitly referenced objects addresses #9.1
Comment #13
drunken monkeyWow, thanks a lot! Amazing job, that’s really a lot of work you put into this, and it already looks very good!
Also, thanks a lot to Joris for another great, thorough review! Once again, I can just agree with most of your points.
But, of course, once again I have to come nagging and nit-picking … ;)
(I do apologize for the delay, though! Was a hectic few weeks for me. Normally, I try to review new issues/comments once per week, but I don’t always manage.)
I’m pretty sure neither of those is true.
The hook is just deprecated for one specific use (removing items). And I don’t know why you’d think the query alter hook isn’t working when you even added another test for it that passes?
Well, for one thing, as I think you yourself once pointed out to me, the
@var
comment isn’t necessary if you have a proper(ly configured) IDE. In the case of PhpStorm, I think it’s the “Drupal Symfony Bridge” plugin that automatically detects the type?So, then you can also get rid of the variable and have just a single string of method calls, albeit on two or even three lines.
I’m thinking, maybe even a trait would make sense, to be used throughout the module? Adding DI for all classes that support it (not the entity classes, but most others) and providing a handy
dispatch()
short-hand method? (Could maybe even take the class name as parameter and then just take care of event name and event object creation? Kinda like you did for plugin managers. Not sure how usable/clean-looking that would be, though. (Also, see below: if we move the event name constants, this wouldn’t be as practical anymore anyways.))Yes, definitely. See #3024461: Adopt consistent deprecation format for core and contrib deprecation messages.
And yes, I’d also guess the next major version will be 9.x-1.0, so let’s use that.
Also, we’ll need to add a
@deprecated
tag with an identical description to all the hook doc blocks.On the other hand, though, Drupal 9 is now only a year away. There’s lots of contrib modules and custom code out there, so I’m not sure one year is really enough advance notice to remove all our hooks.
Maybe we should just add the events for now and leave the deprecation of all hooks for 9.x, then remove them completely in 10.x? I think I’d feel better with that – not optimal to have two parallel systems for so long, but should neither impact maintenance effort nor performance all that much, I’d say, and I really think removing hooks so quickly would put unnecessary stress on a lot of people.
What do you say?
I don’t see this pattern in Core. Is there any specific reason you think this should be the case?
While I don’t see an immediate reason to sub-class event classes, I also don’t see a particular reason to forbid it. In Drupal, I think the general practice is to keep things open for other developers to do things you wouldn’t have thought about doing (and, yes, potentially shoot themselves in the foot while doing so).
If there is no concrete danger of allowing sub-classing (and having properties just
protected
, notprivate
) here, I’d rather keep adhering to this principle.Apart from this, there are unfortunately a lot of coding standards violations. For some of them, setting up your IDE’s code templates can help a lot. (Just as a tip.)
Most importantly, the first line of most doc blocks (except properties and constants) should start with a third-person verb. I started changing that for some of them, but ran out of time.
One thing I’m not quite sure about is how to best phrase this for event classes – e.g.:
Any preferences? I think I personally like the second one better, but if one of you feels something else is more “standard” we should probably use that. (The third one is used in Core, but is a bit too meaningless for my taste.)
I’m also not sure about the event class names. I don’t think the past-tense verbs at the end are a good idea for class names in general. In addition, I think most event class names end with
*Event
, which seems a sensible pattern to adhere to.So, maybe:
BackendPluginDefinitionsEvent
,FieldTypesMappingEvent
,ItemsPreIndexEvent
/ItemsPostIndexEvent
,QueryPreExecuteEvent
/QueryPostExecuteEvent
,ServerFeaturesEvent
, …?Finally, it seems Core uses a different scheme for documenting event names, namely a single enum-like class for each module/component with constants for all events – see, for instance,
\Drupal\Core\Config\ConfigEvents
.While I personally do like your style better, I think we should probably keep consistent with Core’s here, to make it easier for other developers. (Also, in any case, it seems we should use the
@Event
tag to document event names.)(I guess this will necessitate using two arguments for the new plugin manager
alterEvent()
method.)In any case, thanks a lot again for all the work you put into this! I hope we can finalize this soon. (I’ll try to be more responsive in the future.)
Comment #15
borisson_There hasn't been a final call about this, but in theory 8.x-1.x should continue to work on drupal 9. I am not sure what the proposed new scheme for drupal modules and their releases will be. It means I'm not sure if the next version will be 9.x-1.0 or if it will be simply 2.0.
I'd suggest releasing that at the time 9.1 comes out (so in ~18 months). Let's take that discussion to another issue though ;)
This is not a pattern in core, that is true. It is my preferred way of doing this in symfony/laravel projects and I see this a lot in other symfony packages. I don't think there is any reason to ever subclass an event. If you need other information - it is no longer that event but a new one and signalling that with a complete new class is better - I think.
So +1 on the final classes from me here.
Again, I'm going to agree with @legolasbo here. An event is something that happened where the developers can react to. I like the class names they are now, but less strong opinion here than on the classes being final. I also don't really like suffixing things with what they are as much. But I guess we also do that for controllers and such.
Not too fussed about this one. I think the consistency weighs harder here for discoverability, this one can be changed.
Comment #16
drunken monkeyAh, OK, wasn’t aware of that. I thought there would still be two versions, even if they’re almost identical. (Moreover, this will probably even be necessary, as 8.8 and 9.0 will be officially supported at the same time, and deprecations in 8.9 will already be removed in 9.0.)
We can call it “next major version” instead, if you both prefer, but I think just calling it 9.x-1.0 should also be fine. Developers should know what it means and not be surprised if the new version for D9 does end up having a different version number.
What about testing? Pretty sure you can’t get a mock of a final class. (As events should only be data objects, with no logic, I admit the need to mock them will rarely arise, though.)
But also, it’s coming back to the argument of letting people shoot themselves in the foot. We can’t think of all the possible reasons for doing something, and Drupal usually takes the stance of just permitting (programmatically) everything that doesn’t pose a direct, obvious risk – which I can’t really see here. Not sure what the harm would be if someone would trigger an event with a sub-classed event object (even though it might not be “clean”).
On the other hand, later just removing the
final
keyword again should be 100% risk-free, so I guess we can just leave it there and see if someone complains, or we find a good reason to remove it.I yield to the majority. ;)
I don’t really like the suffix thing, either, but it’s undeniably the pattern in Core. See the attached list of all the event class names I could find in Core – only a single one doesn’t end with
*Event
. And as I’m a big fan of standards, I find “Core does it that way” the most important argument in most cases. (Except where Core is really just being stupid.)Also, none of them uses a past tense verb. Which is something I have to disagree on with you, too: in my interpretation, an event is not something that happened, but something that is currently happening. The gathering of the plugin information (for instance) is still ongoing, as event listeners are of course free to change the collected definitions (that’s what they’re for). So the past tense just seems off to me.
So, my personal taste and consistency with Core even align in this case.
Comment #17
borisson_This is a good argument. +1 for renaming them.
Comment #18
legolasboPoor wording from my side. The hook is invoked, but the test did not test for any measurable effect, which Is why I added another test that obviously passes because the hook is invoked and causes the measurable effect. I'll also add an event for hook_search_api_index_items_alter.
I've got the Drupal Symfony Bridge plugin installed, but the fact that we're able to do something doesn't mean that we should do it. Stringing method calls together hides a code smell, which is that we're accessing hidden dependencies. I like those code smells to be painfully obvious to give us a reason to refactor and actually remove the problem (being hidden dependencies).
I think that trait would become overly complex rather quickly, especially if you want to make it responsible for event creation. I'm also failing to see how adding a trait would add DI for all classes that support it? Dependencies should only be injected through the constructor and then never change for the instantiated object, all other methods of injecting dependencies (through setter methods and such) smells of terrible bugs like temporal coupling, side effects, etc.
Agreed, let's just call it 9.x-1.0 for now.
Events should be very specific and only used in a single place, where they are dispatched in a controlled manner. They shouldn't change or be extended, because any changes to the event would actually mean that there is a new event to dispatch.
I'll change this to match core's approach.
I'll change them ;)
Comment #19
drunken monkeyReading through #3055775: Remove explicit references to implicitly referenced objects, I also realize that the new events that are pegged to replace the results alter hooks won’t be completely equivalent, as it’s currently possible to completely replace the result set object, which the new even wouldn’t allow.
While it’s definitely a niche use case, I feel that we should keep supporting this, so the event should have a setter for the result set and we should replace
$this->results
with the result set from the event after triggering it.I’m not really sure what you mean by that? How is that code smell? I think the only thing “smelling” is the use of
\Drupal::getContainer()
/\Drupal::service()
, which should normally be replaced by DI. I don’t see how having that in a single line instead of as a variable hides that at all?Yes, where possible (it isn’t always), getting a service that way should be replaced by DI. But we should just do that here, instead of trying to make it obvious that we’re doing it wrong.
As we’re using the getter/setter method and initialization in
create()
throughout the module, I of course disagree with you there. And, for consistency’s sake, I’d also prefer it this way here. Having a trait for that seems a nice way to do this. All classes with acreate()
method could then set the dispatcher there, while others would just use it as a short-hand for getting the displatcher via\Drupal
(as some classes, like entity classes, can’t properly use DI, unfortunately).I also don’t really see the problem with the method becoming “overly complex”? For one, it would still be just a few lines, but more importantly: isn’t that what traits are for? You have more code in a single place so that all places that need that functionality just have a small method call. As said, though, with the new place for event names, event creation wouldn’t really make sense in the trait anyways, so the
dispatch()
(ordispatchEvent()
?) helper method would just be a one-liner.Comment #20
legolasboYour interdiff from #13 didn't apply cleanly and #13 didn't really match what we've discussed afterwards anymore, so I went through it and made the changes that were still relevant manually. I also renamed all the events and introduced the SearchApiEvents class which contains a list of constants with the event names (those still need to be documented). I also removed all of the specific ***Gathered events in favor of just re-using the base event with different names.
Attached patch contains all the work I got done on this today. Unfortunately I'm probably not able to continue work on this until next week.
Comment #21
legolasboI think I've processed all feedback now. Ready for another round.
Comment #22
drunken monkeyThanks a lot, great progress!
This seems like an unnecessary complications of the code, changes current behavior and is, really, unrelated.
Please create a new issue if you feel like this would be an important change, but as Core doesn’t do this, either (unless I’m mistaken), I really don’t think we should.
If someone really wants to add a new tag within an alter hook, they can still just invoke the hook and dispatch the event themselves.
Apart from this, I went through the code and corrected coding standards issues and the deprecation messages. (It seems the module handler’s
*Deprecated()
methods will already include the hook name in their message, so it’s not needed in our description after all. I just based them on the ones already in Core instead, plus the current proposal in #3024461: Adopt consistent deprecation format for core and contrib deprecation messages. I then added those deprecation messages to the hook documentations as well.I also already added and linked the change record for this issue. Will just have to remember to also publish it once we commit this issue. (We can then still discuss whether to remove hooks right away or wait a few more years after all. But better to warn users right away that the change is coming, and rather state the deadline as too soon than too late.)
Suggestions/corrections for the CR very welcome, of course!
Then, I found two additional hooks which weren’t converted yet, so I added events for those as well. (As their hooks weren’t tested, I didn’t add tests for the new events, either.)
The events are now also properly documented in
SearchApiEvents
.Otherwise, I don’t really have any complaints anymore. After reviews from, ideally, both of you, this would be RTBC in my opinion.
Again, thanks a lot for the great work here!
Comment #24
legolasboIt is what was needed to make the tests pass and since the events tests is now literally the same as the hooks tests (except for the comments) either the hooks implementation does something to take the tags into account, or the hooks test is wrong.
It also looks like your patch contains lots of unrelated changes. I guess something went wrong in it's creation. I did take a look at the interdiff though:
2 doc comments?
+1! That's a way better name
I think this comment is misleading, because nothing is actually checked.
This seems wrong and unrelated.
Comment #25
drunken monkeyWhich test is that? (Will probably see in a moment anyways, though.)
As you saw yourself, the previous implementation definitely didn’t take newly added tags into account, and I don’t know why the test should assume that. As you also saw, the tests were passing fine until now.
But maybe I just don’t understand where the problem is. Might become clearer once I see the failing test. (Now hidden by the myriad of test fails due to the faulty service definition …)
Ah, yes, sorry! In such complex issues, I use a separate branch for developing, making interdiffs, etc., and apparently I
diff
ed against 8.x-1.x instead of the merge-base.Should be fixed in this new patch.
Thanks also for your other comments, these should be fixed now as well!
Comment #27
drunken monkeyAh, now I see. Yes, that hook was previously neither invoked nor tested in
HooksTest
. (It is invoked/tested inQueryTest
instead.) You added testing for it and of course that failed. Anyways, easy to fix and still keep the testing.Also hopefully now fixed all left-over wrong instantiations, without event dispatcher.
Comment #29
drunken monkeyComment #30
borisson_Nipicks ahead!
Shouldn't we be using
alterDeprecated
here?Is there a way to also do the deprecated version of this? I couldn't find a way to do that. So maybe just ignore me.
I'm not sure about the wraps here. Some other events have provides/defines instead and I feel that is a better wording. If you disagree though, feel free to ignore that and change other event to also be Wraps, so we at least have some consistency.
^
^
^
^
^
^
^
Comment #31
drunken monkeyIn a test? Not really sure, to be honest. I’d have felt it would be weird, and any implementations would be found by the other tests (running proper indexing) anyways. But fine to go either way. We’d just hav to duplicate the deprecation message, so that would be awkward, too.
But by the way, is there any way to tell the
HooksTest
that we don’t care about the deprecations in this one? (There areDeprecationListenerTrait
andExpectDeprecationTrait
, but they are both explicitly marked@internal
.)That only sets the hook name to use, it doesn’t actually alter anything. The altering is done in
SearchApiPluginManager::alterDefinitions()
, which is why we (that is, Legolasbo) overrode it.Not quite sure, either, I’m not happy with any of the options, really. However, “wraps” seems to be the most used in Core (albeit with a small sample size, as Core just defines 16 events as far as I can see). A little bash fu got me this list:
Also, our only event whose doc comment didn’t start with “Wraps” was for the
GatheringPluginInfoEvent
, and as that’s used for multiple events I found it might be more appropriate there. But I can also change it to use “Wraps”, if you prefer that. As said, I’m not really very happy with any of these myself.Comment #32
borisson_Oh didn't notice that was in a test. Not that's fine.
Not sure about a way to ignore deprecations.
About the message names - I guess using wraps is a good idea. Let's keep it at that and change the other one for consisteny
Comment #33
drunken monkeyAlright.
Comment #34
borisson_Yup, I like consistency! This now makes a ton of sense.
Comment #36
drunken monkeyAlright, thanks for reviewing again! Good to hear this is now RTBC.
Committed.
Thanks a lot again, both of you!
Comment #38
Kristen PolPer a Slack discussion with Gábor Hojtsy regarding usage of D9 tags (Drupal 9, Drupal 9 compatibility, Drupal 9 readiness, etc.), "Drupal 9 compatibility" should be used for contributed projects that need updating and "Drupal 9" was the old tag for D8 issues before the D9 branch was ready. Doing tag cleanup here based on that discussion.