netgen / NetgenInformationCollectionBundle

Information collection alike feature for eZ Publish 5/eZ Platform
https://netgen.io
GNU General Public License v2.0
19 stars 8 forks source link

NGSTACK-570 add interfaces, modify typehints #76

Closed leohajder closed 2 years ago

leohajder commented 2 years ago

Add interfaces and modify typehints to enable easy service injection. Typehint to interfaces instead of classes.

emodric commented 2 years ago

Adding interfaces for the sake of adding interfaces seems counterproductive to me. It only complicates reading the code.

We do not expect ever having a different InformationCollected class. When and if we do, then we can add an interface, not before.

As for FactoryInterface, adding an empty interface also does not make sense. Interfaces are supposed to define what a class does, or rather, what features it supports. An empty interface tells us nothing.

Btw, how does adding interfaces enable easy service injection? :)

leohajder commented 2 years ago

Ok, sure. Do you have any input on how to solve these and similar errors:

Class Netgen\Bundle\InformationCollectionAkismetIntegrationBundle\InformationCollection\Action\EmailAction requires \Netgen\Bundle\InformationCollectionBundle\Factory\EmailDataFactory, Netgen\Bundle\InformationCollectionAkismetIntegrationBundle\InformationCollection\Factory\EmailDataFactory given

when trying to override NetgenInformationCollectionBundle services in another custom bundle (NetgenInformationCollectionAkismetIntegrationBundle in this case)?

Also, NetgenInformationCollectionBundle already has empty interfaces for typehinting purposes.

leohajder commented 2 years ago

In another project, there are custom infocollector actions, and some are created just to substitute the action data factory. So that results in having to copy and paste the whole action code just to change the typehint on the data factory class, and change the config to use the custom action, etc. Wouldn't it be cleaner if you could just override the data factory service and be done with it? So, having interfaces enables easier service injection, or override I should have said.

leohajder commented 2 years ago

And finally, yes we do expect to have DelayedInformationCollected class in NetgenInformationCollectionAkismetIntegrationBundle. Maybe I should have started with that.

I've integrated the Akismet anti-spam API into infocollector, needed for several projects:

So I've created NetgenInformationCollectionAkismetIntegrationBundle which overrides the necessary sevices and introcuces the DeleyedInformationCollected event, which is dispatched on marking a submission as safe.

Maybe this makes things clearer.

emodric commented 2 years ago

Let me get back to you on this tomorrow 🙂

emodric commented 2 years ago

Thanks @leohajder !