Sending MP with the service about fosmessagebundle HOT 7 CLOSED

Are you using the ODM or ORM?

from fosmessagebundle.

I am using the ORM.

from fosmessagebundle.

Will you fix the bug ?

from fosmessagebundle.

I believe this issue has been fixed since the time it was reported. Please reopen this issue if it still occurs.

from fosmessagebundle.

No, it isn't!

from fosmessagebundle.

I just tested this against the master branch. It does seem to work (tested with the orm implementation)

There is another bug in it though. If you send a message programatically then you won't get redirected to the message itself. That redirection marks your message as seen. Now that doesn't happen and you'll end up with new messages notifications every time you send a message programatically.

Now imagine you want to send every new user a welcome message. It wouldn't be very fun to see your unread messages increase every time you do that. Only to find out that unseen message isn't even in your inbox but in your sentbox.

This also made me inspect the code a lot more and there is a major problem with the way the objects get created. There is no real builder

https://github.com/FriendsOfSymfony/FOSMessageBundle/blob/master/MessageBuilder/NewThreadMessageBuilder.php

This class does it's job very very poorly. It doesn't create a new thread object, it just sets the most basic stuff and then it's up to the rest of the classes to make things work.

An example... If you create a thread, you should create a date when the thread is created. Does that happen there? No

Then where does that happen?
After a lot of looking around it happens in:

https://github.com/FriendsOfSymfony/FOSMessageBundle/blob/master/EntityManager/ThreadManager.php#L373

* Ensures that the createdBy & createdAt properties are set
*/
    protected function doCreatedByAndAt(ThreadInterface $thread)
    {
        if (!($message = $thread->getFirstMessage())) {
            return;
        }

        if (!$thread->getCreatedAt()) {
            $thread->setCreatedAt($message->getCreatedAt());
        }

        if (!$thread->getCreatedBy()) {
            $thread->setCreatedBy($message->getSender());
        }
    }

So let's examine that code. Basicly what it does is the job of the NewthreadMessageBuilder,
It sets the creation date of the thread. But it does more. It RELIES on the fact that the message->getcreatedAt is set. Now another question is... where did that get set?

So what to do about all this?
IMO the NewThreadMessageBuilder should get a lot more responsabilities. It has to build a thread with a message.

Now back to my bug. Who should set that the sender of a threat has already seen the threat. The newthreadmessagebuilder! Where do I have to fix this now? I have no clue...

Just my two cents
@merk @stof

from fosmessagebundle.

@Miliooo really great feedback, thanks! I'll keep it in mind for the new version.

from fosmessagebundle.