dereuromark/cakephp-queue

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

Closed this issue · 16 comments

mrothauer commented

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

Both ways using direct objects have indeed been removed in 4b5b4e7#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

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

what do you think? Does that work?

dereuromark commented

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

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

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

And thank you for your quick reply!

dereuromark commented

Can you check #397 maybe? See docs added

mrothauer commented

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

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

dereuromark commented

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

mrothauer commented

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

No, unfortunately it's not working.

  • Here you can see my working code on CakePHP 5 (develop branch)

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

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

mrothauer commented

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

dereuromark commented

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

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

I would probably try the JSON serialize instead

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

We can chat tomorrow.

mrothauer commented

I would probably try the JSON serialize instead

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

not working

We can chat tomorrow.

cool, I sent you an email.