Skip to content

Patch run_callbacks instead#5

Merged
adrianna-chang-shopify merged 2 commits into
mainfrom
ac-fix-callback-methods
Oct 24, 2025
Merged

Patch run_callbacks instead#5
adrianna-chang-shopify merged 2 commits into
mainfrom
ac-fix-callback-methods

Conversation

@adrianna-chang-shopify

@adrianna-chang-shopify adrianna-chang-shopify commented Oct 22, 2025

Copy link
Copy Markdown

Handle changes to Rails' callback code (rails/rails@207a254), which introduces a "fast path" for callbacks and no-ops the _run_#{callback}_callbacks methods up until the point where a callback is actually defined, at which point Rails aliases _run_#{callback}_callbacks to a ! version of the method.

Rather than patching the ! version of the callbacks, we patch the run_callbacks method. This way, we don't need to worry about callbacks potentially being defined before we include the gem, which can lead to Rails aliasing the non-patched version of the _run_#{callback}_callbacks! methods.

Plus, run_callbacks is public API, so it's better to patch this anyways.

@shopify-shipitnext

Copy link
Copy Markdown

🔎 View this PR in Shipit Next.

ℹ️ Expand to learn how to deploy and handle emergencies using Shipit Next

Overview

Shipit Next will merge your code on your behalf because this repository uses Shipit Next and its merge queue.

To ship this PR, you can either:

Comment Commands

  • /shipit: Enqueue this PR into the merge queue where it will eventually be merged and deployed.
  • /cancel: Eject this PR from the merge queue and rebuild PRs that were enqueued after this PR.
  • /shipit --jump-queue: Enqueue this PR at the top of the merge queue where it will be included in the next deploy. Use this for non-emergency situations.
    - Emergency handling procedure for this command can be found here.
  • /shipit --emergency: Merge this PR directly into main and deploy to all environments once all require_for_emergency CI checks pass. Please be aware that changes deployed with this command will not be automatically rolled back.

Commands exclusive to Deploy Before Merge

  • /cancel --emergency: Eject this PR from the merge and rollback any deployments containing this PR.

Documentation

Questions or feedback?

Rather than patching the `!` version of the callbacks, we patch the `run_callbacks` method.
This way, we don't need to worry about callbacks potentially being defined _before_ we include the gem,
which can lead to Rails aliasing the non-patched version of the `_run_#{callback}_callbacks!` methods.

Plus, `run_callbacks` is public API, so it's better to patch this anyways.
@adrianna-chang-shopify adrianna-chang-shopify force-pushed the ac-fix-callback-methods branch 3 times, most recently from 7f2092d to a8e8754 Compare October 23, 2025 20:07
Previously, these methods would've previously reset the transaction_changed_attributes.
Now, the no-op version *won't* call `run_callbacks` at all, so we should
ensure we still reset the transaction_changed_attributes.

@Edouard-chin Edouard-chin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice work 🥇

@adrianna-chang-shopify adrianna-chang-shopify merged commit e85abfa into main Oct 24, 2025
30 checks passed
@adrianna-chang-shopify adrianna-chang-shopify deleted the ac-fix-callback-methods branch October 24, 2025 19:35
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.

2 participants