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

There is a bug in payload transmission #33

Closed
wants to merge 2 commits into from

Conversation

igor875126
Copy link
Contributor

Hey, I've found a behavior that when you use not an wildcard listener the payload is given as string in the listener handle method, this string is the first element of payload array.

But if you wrap it in the additional array, then everything is working as expected, wildcard listeners tested too....

Example:

BroadcastEventServiceProvider

<?php

namespace App\Providers;

use App\Listeners\UserAuthenticatedEvent\UserAuthenticatedListener;

class BroadcastEventServiceProvider extends \Nuwber\Events\BroadcastEventServiceProvider
{
    protected $listen = [
        'gateway.user.authenticated' => [
            UserAuthenticatedListener::class
        ],
    ];
}

UserAuthenticatedListener

<?php

namespace App\Listeners\UserAuthenticatedEvent;

class UserAuthenticatedListener
{
    /**
     * This method is called when event happens
     * see app/Providers/BroadcastEventServiceProvider.php
     *
     * @param $payload
     */
    public function handle($payload)
    {
        dd($payload);
    }
}

Payload in rabbitmq message

{"event_id":"254","fingerprint":"c0feace7d38377bff938b590b3eb5e181d858381","user_agent":"PostmanRuntime\/7.17.1","user_id":11114022,"merchant_id":-5,"ip_address":"172.18.0.1"}

dd($payload)

"254"

With my fix - dd($payload)

array:6 [
  "event_id" => "254"
  "fingerprint" => "c0feace7d38377bff938b590b3eb5e181d858381"
  "user_agent" => "PostmanRuntime/7.17.1"
  "user_id" => 11114022
  "merchant_id" => -5
  "ip_address" => "172.18.0.1"
]

BTW: I am using PHP 7.3.9 + Laravel ^6.0
nuwber/rabbitevents 2.0 - same behavoir
nuwber/rabbitevents 3.0-beta1 - same behavoir

@igor875126 igor875126 changed the title There is a bug in payload transimission There is a bug in payload transmission Sep 27, 2019
@masterjus
Copy link
Contributor

Hi @igor875126
I know that. It is mentioned in the documentation. You need to wrap your payload because it passes to a listener with call_user_func_array. Because of this, I've made new functionality with publish helper and trait. Please try it.

I'll be glad to get feedback from you if it works or not.

@igor875126
Copy link
Contributor Author

Hi @igor875126
I know that. It is mentioned in the documentation. You need to wrap your payload because it passes to a listener with call_user_func_array. Because of this, I've made new functionality with publish helper and trait. Please try it.

I'll be glad to get feedback from you if it works or not.

Hey,

thank you for your answer, unfortunately i cannot do this, because of microservice architecture and other libraries. I mean in the project where payload is beeing sent we are using another framework and another rabbit lib.

Please check my merge request, I am almost 100% sure that this will work with even not surrounded payloads.

Copy link
Contributor

@masterjus masterjus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My project is working on rabbitevents and we're sending all our requests like fire('event.name', ['arg1', ['key' => 'value'], ... ]). It wouldn't work if I apply this change.
Second thing is that tests are not passed. Please see my code comment.

@@ -59,7 +59,7 @@ public function __construct(
*/
public function fire()
{
return call_user_func($this->listener, $this->event, Arr::wrap($this->payload()));
return call_user_func($this->listener, $this->event, Arr::wrap([$this->payload()]));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. In this case, you don't need Arr::wrap
  2. What if somebody passed data covered by []? Already working applications must became broken.

@masterjus
Copy link
Contributor

@igor875126 is this PR still actual?

@igor875126
Copy link
Contributor Author

@igor875126 is this PR still actual?

No, it's not actual any more. You can close it. Unfortunately, we are forced to fork your repository and continue developing to our needs.

@igor875126 igor875126 closed this Oct 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants