Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Can't extend sender object #22

Open
iBasit opened this issue Jan 28, 2016 · 9 comments
Open

Can't extend sender object #22

iBasit opened this issue Jan 28, 2016 · 9 comments

Comments

@iBasit
Copy link

iBasit commented Jan 28, 2016

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
Copy link
Contributor

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
Copy link
Contributor

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

@tgalopin
Copy link
Contributor

tgalopin commented Feb 1, 2016

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
Copy link
Contributor

tgalopin commented Feb 1, 2016

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
Copy link
Author

iBasit commented Feb 1, 2016

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

  • attached messaging system with firebase to make it live chatting.
  • will add email message on top. (pending)

@tgalopin
Copy link
Contributor

tgalopin commented Feb 1, 2016

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
Copy link
Author

iBasit commented Feb 1, 2016

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

@tgalopin
Copy link
Contributor

tgalopin commented Feb 1, 2016

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
Copy link
Author

iBasit commented Feb 2, 2016

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants