dereuromark / cakephp-queue

Queue plugin for CakePHP - simple, pure PHP and without dependencies.
MIT License
306 stars 137 forks source link

EmailTask with ['settings' => $this->getMessage()] not working any more #396

Closed mrothauer closed 8 months ago

mrothauer commented 9 months ago

Hi,

I just migrated my app to CakePHP 5 / CakePHP-Queue v7 and I am missing the option to pass a MessageObject to the EmailTask like this:

$queuedJobs->createJob('EmailTask', [
     'settings' => $this->getMessage(),
]);

I am missing this part: https://github.com/dereuromark/cakephp-queue/blob/cake4/src/Queue/Task/EmailTask.php#L137-L159

In the docs I found this that might work: But how would I pass the message?

$data = [
    'class' => Message::class,
    'settings' => $settings,
];
$queuedJobsTable = TableRegistry::getTableLocator()->get('Queue.QueuedJobs');
$queuedJobsTable->createJob('Queue.Email', $data);

Or is there another solution / workaround to pass the whole MessageObject I did not see?

dereuromark commented 9 months ago

Both ways using direct objects have indeed been removed in https://github.com/dereuromark/cakephp-queue/commit/4b5b4e70486a7af66d77272a76dcd0d4243a916f#diff-4e1330cc87ebdba754ed6e65cdff7eccbc9fcd17102846c9b28327ad3a6c07d5L116-L135

The main issue with a whole object is that this is very easy to break during deploys. A single small change and your current queue is broken beyond repair.

So the recommendation is to use config only, and never store serialized objects in the queue. Are you using json or php serialize by the way?

The new (recommended) way using JSON would not work with it anyway afaik, so here we would need to always use the config way.

As for your issue then: You can

what do you think? Does that work?

dereuromark commented 9 months ago

Looking into the code we could support passing the whole object and internally do

__serialize()

and later then __unserialize() with that data array.

Would you want to make a PR to support passing a full array here?

It probably would then be

'class' => Message::class,
'settings' => serialize($this->getMessage()),
'serialized' => true,

And then on the other side, if serialized is set, using createFromArray() to build it again.

mrothauer commented 9 months ago

The main issue with a whole object is that this is very easy to break during deploys. A single small change and your current queue is broken beyond repair.

Ok I understand that (although I never had problems with it within the last years I was using it that way)

So the recommendation is to use config only, and never store serialized objects in the queue. Are you using json or php serialize by the way?

I am using php serialize.

  • copy the old EmailTask to project level and continue using it (Email instead of Queue.Email).

That is my current solution, but I prefer to use maintained code from other repos :-)

mrothauer commented 9 months ago

Looking into the code we could support passing the whole object and internally do

__serialize()

and later then __unserialize() with that data array.

Sounds great.

Would you want to make a PR to support passing a full array here?

Sorry, I have no time for that at the moment. I took me a while to report my problem :-)

It probably would then be

'class' => Message::class,
'settings' => serialize($this->getMessage()),
'serialized' => true,

And then on the other side, if serialized is set, using createFromArray() to build it again.

Ok

mrothauer commented 9 months ago

And thank you for your quick reply!

dereuromark commented 9 months ago

Can you check https://github.com/dereuromark/cakephp-queue/pull/397 maybe? See docs added

mrothauer commented 9 months ago

Please tell me when you discussion in the PR has finished. My goal is to be able to use the queue for sending emails as simple as possible. With the solution I was working so far (passing the Message Object) I had no troubles with attachments and any other fields that the Message might have.

dereuromark commented 9 months ago

Where do you assemble your email? What kind of code are you using?

dereuromark commented 9 months ago

Also, did you test my PR? does it work for you?

mrothauer commented 9 months ago

Also, did you test my PR? does it work for you?

No, unfortunately it's not working.

https://github.com/foodcoopshop/foodcoopshop/blob/develop/src/Mailer/AppMailer.php https://github.com/foodcoopshop/foodcoopshop/blob/develop/src/Queue/Task/AppEmailTask.php

AppEmail Task on develop does not extend EmailTask, here I copied the part that did not make it in Queue v7

I hope that helps.

mrothauer commented 9 months ago

And if it's too much work for you, I just continue with the copied code. It's not that much code.

mrothauer commented 9 months ago

If you want to experiment on my code basis, CartsControllerTest::testFinishOrderWithComment covers that part. If it passes, your changes are working.

dereuromark commented 9 months ago

No, unfortunately it's not working.

I am interested in how you coded this What did you try?

Just so I can more easily try to reproduce what went wrong.

mrothauer commented 9 months ago

I changed the call in AppMailer to

        $queuedJobs->createJob('AppEmail', [
            'class' => Message::class,
            'settings' => serialize($this->getMessage()),
            'serialized' => true,
            'afterRunParams' => $this->afterRunParams,
        ]);

and then reverted my AppEmailTask to the version that extends EmailTask (without my manually inserted code from Queue v6)

and than run my test => it failed

1) CartsControllerTest::testFinishOrderWithComment
Failed asserting that 'Bestellbestätigung' is in an email subject
was: .

Maybe it's quicker to talk? https://meet.jit.si/rothauer-it

dereuromark commented 9 months ago

I would probably try the JSON serialize instead

$this->getMessage()->__serialize()

We can chat tomorrow.

mrothauer commented 9 months ago

I would probably try the JSON serialize instead

$this->getMessage()->__serialize()

not working

We can chat tomorrow.

cool, I sent you an email.