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

Fix mailer argument deserialisation for 6.1 on Ruby 3.1 #2570

Merged
merged 3 commits into from Mar 14, 2022

Conversation

pirj
Copy link
Member

@pirj pirj commented Jan 28, 2022

I've cherry-picked #2566 to main.
Locally,on Ruby 3.1 it passed for 6.0 (to fix failing sub-builds) and for 7.0, too.

@pirj pirj self-assigned this Jan 28, 2022
@pirj
Copy link
Member Author

pirj commented Jan 29, 2022

Without fixes from #2552, 7.0 builds expectedly fail.

What fails on 6.1 and 6.0 are those 2 scenarios.
It seems it needs more specs for various combinations depending on self.delivery_job = ActionMailer::MailDeliveryJob setting like even the same Rails version can have a different global or even a mailer-local setting for a job that affects serialization, e.g. mentioned here, and in our own specs:

    class UnifiedMailer < ActionMailer::Base
      self.delivery_job = ActionMailer::MailDeliveryJob
# vs
      self.delivery_job = ActionMailer::DeliveryJob

I can hack on it next week.

My impression is that we won't be able to make enuque_mail to match with both with({'foo' => 'bar'}, 1, 2) and with(params: {'foo' => 'bar'}, args: [1, 2]), so this will be a breaking change. I admit recklessly merging #2546 without noticing this change of behaviour.

@JonRowe JonRowe force-pushed the fix-ruby-31-enqueue_mail-matcher branch from 423ea20 to 6fd11ab Compare March 7, 2022 11:04
@JonRowe JonRowe force-pushed the fix-ruby-31-enqueue_mail-matcher branch from 5174152 to b22a08b Compare March 14, 2022 10:13
@JonRowe JonRowe merged commit 5fe75e7 into main Mar 14, 2022
@JonRowe JonRowe deleted the fix-ruby-31-enqueue_mail-matcher branch March 14, 2022 10:43
@JonRowe
Copy link
Member

JonRowe commented Mar 14, 2022

@pirj I've merged this because I'm reconcilling the main branch with the released 5.1.1, my intent is to ressurrect the discussion around the changed behaviour in another branch on top of this

csutter added a commit to DFE-Digital/teaching-vacancies that referenced this pull request May 25, 2022
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