FriendsOfSymfony / FOSMessage

Provides user-to-user messaging features for PHP applications.
MIT License
63 stars 16 forks source link

Can't extend sender object #22

Open iBasit opened 8 years ago

iBasit commented 8 years ago

I wanted to extend the sender object to overwrite existing methods to match with our business specs.But I can't do, because lots of methods and properties are private. can you please change it to protected so it is easier to overwrite.

I can re-write that to have same effect, but it was best that if it can be avoided.

tgalopin commented 8 years ago

That's a good idea. I don't have time right now, I'll do it this night. If you need it now, please feel free ot open a PR :) !

tgalopin commented 8 years ago

I would also do it for the Repository and the Tagger btw.

tgalopin commented 8 years ago

I thought a bit about this and I'm not sure it's a good idea after all. I think a complete event system should be enough to do anything want. I'll improve the current event system. Tell me if you don't have anything you need.

tgalopin commented 8 years ago

I changed a bit the events, for more precision in targeting a given event. Instead of general PRE_PERSIST and POST_PERSIST, you have now 4 events: https://github.com/FriendsOfSymfony/FOSMessage/blob/master/src/EventDispatcher/EventDispatcherInterface.php

I know this will probably break your code but it's quite easy to fix: just use these constants instead of the old ones. You can check the Sender if you want to know when these events are dispatched: https://github.com/FriendsOfSymfony/FOSMessage/blob/master/src/Sender.php#L54.

iBasit commented 8 years ago

Well honestly, I think adding extra event to make precision targeting is much better solution and improvement.

But it wont hurt to have private methods or properties changed to protected. It gives full flexibility, just for sake of it.

When I overwritten sender class, it gave me couple of issues. So I had to copy all of it, which I didn't wanted to do. but at the moment messaging system is done for us. Using our sender class for now (recommend others to use events).

tgalopin commented 8 years ago

But it wont hurt to have private methods or properties changed to protected. It gives full flexibility, just for sake of it.

Actually, the problem is that once I declare a property or a method as protected, I need to keep it until the next major version as I'm not allowed to BC break in minor ones. That's why I'm not so much a fan of setting properties as protected.

Is there something you currently can't do with the event system? It's a much better solution for extendability.

iBasit commented 8 years ago

I can't really think of anything that event can't do.

tgalopin commented 8 years ago

I think I'll create protected getters in the sender. I'll think about it to do something useful but still maintainable.

Anyway, thanks very much for your feedbacks! If you are interested, I made a demo application using Symfony 2.8 and FOSMessage: https://github.com/tgalopin/FOSMessage-demo. It let me see how we could create a useful bundle in the future.

iBasit commented 8 years ago

If you do create that, which will also be a plus point.

But you will also have to make other methods protected as they are private, which caused our code issues, so I had to also copy private methods.