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:

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.)

CommentFileSizeAuthor
#33 3023704-33--convert_hooks_to_events.patch93.21 KBdrunken monkey
#33 3023704-33--convert_hooks_to_events--interdiff.txt475 bytesdrunken monkey
#29 3023704-29--convert_hooks_to_events.patch93.24 KBdrunken monkey
#29 3023704-29--convert_hooks_to_events--interdiff.txt602 bytesdrunken monkey
#27 3023704-27--convert_hooks_to_events.patch92.86 KBdrunken monkey
#27 3023704-27--convert_hooks_to_events--interdiff.txt3.31 KBdrunken monkey
#25 3023704-25--convert_hooks_to_events.patch90.92 KBdrunken monkey
#25 3023704-25--convert_hooks_to_events--interdiff.txt1.39 KBdrunken monkey
#22 3023704-22--convert_hooks_to_events.patch123.87 KBdrunken monkey
#22 3023704-22--convert_hooks_to_events--interdiff.txt79.8 KBdrunken monkey
#21 3023704-21.patch57.61 KBlegolasbo
#21 interdiff-3023704-20-21.txt16.6 KBlegolasbo
#20 3023704-20.patch51.16 KBlegolasbo
#20 interdiff-3023704-10-20.txt45.54 KBlegolasbo
#16 event-class-names.txt540 bytesdrunken monkey
#13 3023704-13--convert_hooks_to_events.patch54.46 KBdrunken monkey
#13 3023704-13--convert_hooks_to_events--interdiff.txt23.45 KBdrunken monkey
#10 3023704-10.patch52.59 KBlegolasbo
#10 interdiff-8-10.txt43.92 KBlegolasbo
#8 3023704-8.patch56.69 KBlegolasbo
#7 3023704-7.patch56.69 KBlegolasbo
#6 3023704-6.patch56.79 KBlegolasbo
#5 3023704-5.patch48.23 KBlegolasbo
#4 3023704-4.patch38.55 KBlegolasbo
#3 hook-conversion-3023704-3.patch22.69 KBlegolasbo
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

drunken monkey created an issue. See original summary.

drunken monkey’s picture

Note: 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.

legolasbo’s picture

Assigned: Unassigned » legolasbo
Category: Plan » Task
Status: Active » Needs work
FileSize
22.69 KB

I'm taking a stab at this and have attached a first WIP patch. Planning to continue work on this tomorrow.

legolasbo’s picture

Added a bunch more events let's see what testbot thinks about it before I continue working on this later.

legolasbo’s picture

I'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!

legolasbo’s picture

Status: Needs work » Needs review
FileSize
56.79 KB

There, that's all of them! :)

legolasbo’s picture

And that should fix the last coding standards issues.

legolasbo’s picture

Assigned: legolasbo » Unassigned
FileSize
56.69 KB

This should really fix the last one.

borisson_’s picture

Status: Needs review » Needs work
  1. +++ b/search_api.api.php
    @@ -271,7 +271,7 @@ function hook_search_api_items_indexed(\Drupal\search_api\IndexInterface $index,
    -function hook_search_api_query_alter(\Drupal\search_api\Query\QueryInterface &$query) {
    +function hook_search_api_query_alter(\Drupal\search_api\Query\QueryInterface $query) {
    

    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?

  2. +++ b/src/Backend/BackendPluginManager.php
    @@ -28,9 +30,10 @@ class BackendPluginManager extends DefaultPluginManager {
    -    parent::__construct('Plugin/search_api/backend', $namespaces, $module_handler, 'Drupal\search_api\Backend\BackendInterface', 'Drupal\search_api\Annotation\SearchApiBackend');
    ...
    +    parent::__construct('Plugin/search_api/backend', $namespaces, $module_handler, BackendInterface::class, SearchApiBackend::class);
    

    Nice! I like this notation a lot more as well.

  3. +++ b/src/Display/DisplayPluginManager.php
    @@ -32,6 +33,7 @@ class DisplayPluginManager extends DefaultPluginManager implements DisplayPlugin
    +    $this->alterEvent(DisplaysAlter::CLASS);
    

    This should probably be lowercase class.

  4. +++ b/src/Entity/Index.php
    @@ -996,7 +998,13 @@ class Index extends ConfigEntityBase implements IndexInterface {
    +      /** @var \Symfony\Component\EventDispatcher\EventDispatcherInterface $dispatcher */
    +      $dispatcher = \Drupal::getContainer()->get('event_dispatcher');
    +      $dispatcher->dispatch(ItemsIndexed::NAME, new ItemsIndexed($this, $processed_ids));
    
    @@ -1100,7 +1108,11 @@ class Index extends ConfigEntityBase implements IndexInterface {
    +      /** @var \Symfony\Component\EventDispatcher\EventDispatcherInterface $dispatcher */
    +      $dispatcher = \Drupal::getContainer()->get('event_dispatcher');
    +      $dispatcher->dispatch(Reindex::NAME, new Reindex($this, FALSE));
    

    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?

  5. +++ b/src/Event/FieldTypeMappingAlter.php
    @@ -0,0 +1,43 @@
    +  private $fieldTypeMapping;
    

    /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.

  6. +++ b/src/Query/Query.php
    @@ -564,11 +572,17 @@ class Query implements QueryInterface {
    +    $description = sprintf('Use %s instead', ResultsAlter::class);
    +    $this->getModuleHandler()->alterDeprecated($description, $hooks, $this->results);
    

    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.

legolasbo’s picture

  • #9.1: Yes, that was only changed because objects are references by default, but I undid that change and will open a new issue for it.
  • #9.2: It also makes the dependency more visible and explicit because it adds a use statement.
  • #9.3: Good catch, that's been fixed.
  • #9.4: I'd thought about abstracting that, but anything other than dependency injecting it would be less than ideal and because the code already uses several other static methods on the \Drupal class, I chose to follow that pattern for now.
  • #9.5: Events should never be extended so I've made them final instead of going for the protected route.
  • #9.6: I thought about adding more info in the deprecation warning, but came to the conclusion that saying when something got deprecated doesn't really add anything (that's what we've got version control for) and we can't add the next major version number since we don't know what it will be.

Anyway, improved version below :)

legolasbo’s picture

Status: Needs work » Needs review
legolasbo’s picture

drunken monkey’s picture

Wow, 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'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.

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?

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?

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.))

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.

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?

#9.5: Events should never be extended so I've made them final instead of going for the protected route.

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, not private) 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.:

  1. Signals that tracker plugin information is being collected.
  2. Allows altering of tracker plugin definitions.
  3. Wraps information about the tracker plugin definitions event.

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.)

Status: Needs review » Needs work

The last submitted patch, 13: 3023704-13--convert_hooks_to_events.patch, failed testing. View results

borisson_’s picture

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.

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 ;)

#9.5: Events should never be extended so I've made them final instead of going for the protected route.

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, not private) here, I’d rather keep adhering to this principle.

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.

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.

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.

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.)

Not too fussed about this one. I think the consistency weighs harder here for discoverability, this one can be changed.

drunken monkey’s picture

FileSize
540 bytes

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 ;)

Ah, 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.

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.

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. ;)

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.

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.

borisson_’s picture

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.

This is a good argument. +1 for renaming them.

legolasbo’s picture

Assigned: Unassigned » legolasbo
I'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.

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?

Poor 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.

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'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’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.

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.

... discussion over multiple comments ... 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.

Agreed, let's just call it 9.x-1.0 for now.

#9.5: Events should never be extended so I've made them final instead of going for the protected route.

I don’t see this pattern in Core. Is there any specific reason you think this should be the case?

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.

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.)

Not too fussed about this one. I think the consistency weighs harder here for discoverability, this one can be changed.

I'll change this to match core's approach.

... discussion over multiple comments ...

This is a good argument. +1 for renaming them.

I'll change them ;)

drunken monkey’s picture

Reading 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'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’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.

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.

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 a create() 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() (or dispatchEvent()?) helper method would just be a one-liner.

legolasbo’s picture

Assigned: legolasbo » Unassigned
FileSize
45.54 KB
51.16 KB

Your 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.

legolasbo’s picture

Status: Needs work » Needs review
FileSize
16.6 KB
57.61 KB

I think I've processed all feedback now. Ready for another round.

drunken monkey’s picture

Thanks a lot, great progress!

+++ b/src/Query/Query.php
@@ -546,17 +547,36 @@ class Query implements QueryInterface {
+      // We have to do multiple passes because new tags could have been added by
+      // the modules altering the query.
+      do {

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!

Status: Needs review » Needs work

The last submitted patch, 22: 3023704-22--convert_hooks_to_events.patch, failed testing. View results

legolasbo’s picture

+++ b/src/Query/Query.php
@@ -546,17 +547,36 @@ class Query implements QueryInterface {
+      // We have to do multiple passes because new tags could have been added by
+      // the modules altering the query.
+      do {

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.

It 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:

  1. +++ b/src/Display/DisplayPluginManager.php
    @@ -30,8 +30,22 @@ class DisplayPluginManager extends SearchApiPluginManager {
       /**
        * {@inheritdoc}
        */
    -  public function __construct(\Traversable $namespaces, CacheBackendInterface $cache_backend, ModuleHandlerInterface $module_handler) {
    -    parent::__construct('Plugin/search_api/display', $namespaces, $module_handler, 'Drupal\search_api\Display\DisplayInterface', 'Drupal\search_api\Annotation\SearchApiDisplay');
    +  /**
    +   * Constructs a new class instance.
    +   *
    

    2 doc comments?

  2. +++ b/src/Entity/Server.php
    similarity index 76%
    rename from src/Event/IntializingServerFeaturesEvent.php
    
    rename from src/Event/IntializingServerFeaturesEvent.php
    rename to src/Event/DeterminingServerFeaturesEvent.php
    

    +1! That's a way better name

  3. +++ b/src/Event/ReindexScheduledEvent.php
    @@ -48,10 +48,11 @@ public function getIndex() {
    -   * Get the boolean indicating whether the index was also cleared.
    +   * Checks whether the index was also cleared.
    

    I think this comment is misleading, because nothing is actually checked.

  4. +++ b/CHANGELOG.txt
    @@ -1,13 +1,5 @@
    -- #2986222 by idebr, drunken monkey: Switched Javascript tests to
    
    -- #2986222 by idebr, drunken monkey: Switched Javascript tests to
    -  WebDriverTestBase.
    -- #2840272 by drunken monkey: Fixed fatal error in certain Views error
    
    -- #2840272 by drunken monkey: Fixed fatal error in certain Views error
    -  conditions.
    -- #3057510 by drunken monkey, borisson_: Fixed inconsistent package names for
    
    -- #3057510 by drunken monkey, borisson_: Fixed inconsistent package names for
    -  test modules.
    -- #3031621 by drunken monkey, DeFr, borisson_: Fixed fields returned by backend
    
    -- #3031621 by drunken monkey, DeFr, borisson_: Fixed fields returned by backend
    -  sometimes ignored by Views.
    

    This seems wrong and unrelated.

drunken monkey’s picture

It 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.

Which 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 …)

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:

Ah, yes, sorry! In such complex issues, I use a separate branch for developing, making interdiffs, etc., and apparently I diffed 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!

Status: Needs review » Needs work

The last submitted patch, 25: 3023704-25--convert_hooks_to_events.patch, failed testing. View results

drunken monkey’s picture

Ah, now I see. Yes, that hook was previously neither invoked nor tested in HooksTest. (It is invoked/tested in QueryTest 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.

Status: Needs review » Needs work

The last submitted patch, 27: 3023704-27--convert_hooks_to_events.patch, failed testing. View results

drunken monkey’s picture

borisson_’s picture

Status: Needs review » Needs work

Nipicks ahead!

  1. +++ b/modules/search_api_db/tests/src/Kernel/BackendTest.php
    @@ -818,6 +820,9 @@ protected function indexItemDirectly(IndexInterface $index, ItemInterface $item)
         \Drupal::moduleHandler()->alter('search_api_index_items', $index, $items);
    

    Shouldn't we be using alterDeprecated here?

  2. +++ b/src/Backend/BackendPluginManager.php
    @@ -26,11 +29,15 @@ class BackendPluginManager extends DefaultPluginManager {
         $this->alterInfo('search_api_backend_info');
    

    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.

  3. +++ b/src/Event/IndexingItemsEvent.php
    @@ -0,0 +1,70 @@
    + * Wraps an indexing items event.
    

    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.

  4. +++ b/src/Event/ItemsIndexedEvent.php
    @@ -0,0 +1,60 @@
    + * Wraps an items indexed event.
    

    ^

  5. +++ b/src/Event/MappingFieldTypesEvent.php
    @@ -0,0 +1,41 @@
    + * Wraps a field types mapped event.
    

    ^

  6. +++ b/src/Event/MappingViewsFieldHandlersEvent.php
    @@ -0,0 +1,47 @@
    + * Wraps a mapping Views field handlers event.
    

    ^

  7. +++ b/src/Event/MappingViewsHandlersEvent.php
    @@ -0,0 +1,44 @@
    + * Wraps a mapping Views handlers event.
    

    ^

  8. +++ b/src/Event/ProcessingResultsEvent.php
    @@ -0,0 +1,59 @@
    + * Wraps a processing results event.
    

    ^

  9. +++ b/src/Event/QueryPreExecuteEvent.php
    @@ -0,0 +1,40 @@
    + * Wraps a query pre-execute event.
    

    ^

  10. +++ b/src/Event/ReindexScheduledEvent.php
    @@ -0,0 +1,61 @@
    + * Wraps a reindex scheduled event.
    

    ^

drunken monkey’s picture

Status: Needs work » Needs review

Shouldn't we be using alterDeprecated here?

In 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 are DeprecationListenerTrait and ExpectDeprecationTrait, but they are both explicitly marked @internal.)

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.

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.

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.

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:

 * Event fired when rendering main content, to select a page display variant.
 * Block content event to allow setting an access dependency.
 * Event fired when a section component's render array is being built.
 * Event object to allow configuration to be overridden by modules.
 * Provides a class for events related to configuration translation mappers.
 * Wraps a configuration event for event listeners.
 * Represents route building information as event.
 * Wraps a pre- or post-rollback event for event listeners.
 * Wraps a row deletion event for event listeners.
 * Wraps a migrate map delete event for event listeners.
 * Wraps an idmap message event for event listeners.
 * Wraps a migrate map save event for event listeners.
 * Provides a language override event for event listeners.
 * Gets information on all the possible configuration collections.
 * Wraps a configuration event for event listeners.
 * Defines a Locale event.

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.

borisson_’s picture

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

drunken monkey’s picture

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

Alright.

borisson_’s picture

Status: Needs review » Reviewed & tested by the community

Yup, I like consistency! This now makes a ton of sense.

drunken monkey’s picture

Status: Reviewed & tested by the community » Fixed

Alright, thanks for reviewing again! Good to hear this is now RTBC.
Committed.

Thanks a lot again, both of you!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

Kristen Pol’s picture

Issue tags: +Drupal 9 compatibility

Per 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.