search

algolia / scout-extended

Scout Extended: The Full Power of Algolia in Laravel
https://www.algolia.com/doc/framework-integration/laravel/getting-started/introduction-to-scout-extended/
MIT License
393 stars 85 forks source link

Remove final keyword from AggregatorObserver class #312

Closed stevenmaguire closed 1 year ago

stevenmaguire commented 1 year ago
Q A
Bug fix? no
New feature? yes
BC breaks? no
Related Issue
Need Doc update no

Describe your change

Maybe you would consider this a new feature, perhaps not?

Remove final keyword from Algolia\ScoutExtended\Searchable\AggregatorObserver, allowing extension and modification.

What problem is this fixing?

Currently, I have a use case where I want to update the logic of the event handler methods (saved, deleted, forceDeleted). I am able to build a new, customized ModelObserver class that extends this package's ModelObserver class which is nice and expected.

Unfortunately, and for reasons that are not abundantly clear, the package's AggregatorObserver class is decorated with the final keyword which means updating the behavior would require reimplementing the entire class, instead of simply extending and modifying only where needed.

Specifically there is one line of code in the default saved where this is problematic:

https://github.com/algolia/scout-extended/blob/625276ab9ff5b11a22942a5d41063186bcbfaeec/src/Searchable/AggregatorObserver.php#L70-L81

This line of code invokes the parent ModelObserver (which is the default ModelObserver from the package) and there is no way to intercept this invocation to direct the call to a custom ModelObserver.

https://github.com/algolia/scout-extended/blob/625276ab9ff5b11a22942a5d41063186bcbfaeec/src/Searchable/AggregatorObserver.php#L15-L20

Having the ability to replace the implementation registered in the service container with a new AggregatorObserver that extends the package's default AggregatorObserver would be a real nice lift here.

$this->app->singleton(AggregatorObserver::class, MyCustomAggregatorObserver::class);

Please consider one of the two outcomes here:

  1. Let's work together to get this change safely merged
  2. Explain why it is important for the Algolia\ScoutExtended\Searchable\AggregatorObserver class to be final

There is a possibility that this change will unblock https://github.com/algolia/scout-extended/pull/301 as well - downstream implementations could update the proposed logic in that PR on their own.

stevenmaguire commented 1 year ago

It appears these CI checks are failing due to CircleCI configuration issues. I hope that does not hold up a discussion here.

stevenmaguire commented 1 year ago

@DevinCodes @nunomaduro do you have any thoughts here?

DevinCodes commented 1 year ago

Hi @stevenmaguire !

Thank you for this PR, and sorry for the delay on this. Personally, I'm all for removing the final keywords in our classes so people can extend on the functionality. Please note that as soon as someone does that, we're no longer able to provide support given that there are customizations in place. This may be worth it in some cases, though.

There are multiple currently classes that are final, we'll work on a PR soon to remove it on the other classes too (unless you want to include those in this PR as well).

I'll try to take some time this week to work on that PR and a new release!

stevenmaguire commented 1 year ago

Personally, I'm all for removing the final keywords in our classes so people can extend on the functionality.

That is exactly the benefit! The final keyword is useful for sure, and IMO it has limited use cases where it is appropriate. So far, I am not sure those use cases exist in this library.

Please note that as soon as someone does that, we're no longer able to provide support given that there are customizations in place. This may be worth it in some cases, though.

You are still in charge of the behavior of your library. Strangers can't just randomly change your package. If someone forks your package and makes changes - like extending a currently Final class - then yes, of course, I do not think there is any reasonable assumption that becomes your problem too.

There are multiple currently classes that are final, we'll work on a PR soon to remove it on the other classes too (unless you want to include those in this PR as well).

If you'd like to expand the scope of this discussion to become a wide-spread policy for the library, then I am in favor of you moving forward on your own - you know the project in greater detail. Feel free to close this PR if that's how you proceed.

I will welcome a new version of this library that is open for extension!

DevinCodes commented 1 year ago

Closing this in favor of https://github.com/algolia/scout-extended/pull/317, where we remove all final keywords 🙂