active_job is not an optional dependency. We don't support to comment the railtie since other framework depend on it.

Actually, while it is not optional right now, I think it could be. @matthewd do you remember why we didn't make active job optional?

Hi
If you do not think it's a bug, you can close it.
if this change were made on rails 6.0
I would have no problem accepting it.
But in a small version, making a change that breaks things does not make sense to me.
The application has been running since rails 3 and has never used Active Job.
So I thought it was a bug.
Thanks for clarifying.

This absolutely is a bug to introduce a breaking change in a patch version of Rails.

Further, it is preposterous to think that ActiveJob is a required framework. Sure, all of the apps you work on may have a good use case for queued background work, but that just means you're an expert in a particular niche.

Hello,

I don't think it was really @rafaelfranca or any contributors intention to introduce a breaking change in a patch release. This is an unintentional side effect of merging 44133fd in ...

But yeah, as @rafaelfranca pointed out — we didn't make Active Job as an optional dependency, but it just happen to be that you could exclude Active Job and the app was still working.

So, given that (1) you could use Rails without Active Job before 2.5.3 (2) Active Job is not a declared dependency on railtie gem, I think I'm leaning toward fixing that code to be this:

if config.respond_to?(:active_job)
  queue_adapter = config.active_job.queue_adapter

  ActiveSupport.on_load(:active_job) do
    self.queue_adapter = :inline
  end
end

That way we can allow people to still use Rails without Active Job and not introduce an unintended regression as a stop gap. Then, we can properly introduce a test and documentation to sanction that Rails app can run without Active Job if you choose to do so in the future versions.

What do you think about my proposal?

(I really wish we found out about this during RC period ... T_T)

@sikachu, I was considering something like this:

def load_seed
  seed_file = paths["db/seeds.rb"].existent.first
  return unless seed_file

  if config.respond_to?(:active_job)
    with_inline_jobs { load(seed_file) }
  else
    load(seed_file)
  end
end

I'd keep the conditional closer to the usage. @jlw do you want to open a PR?

@rafaelfranca - sure, I'd love to put together a PR on this - but this is my first time working on Rails directly and I imagine it will be Monday at the earliest.

@rafaelfranca Active Job is optional -- but it's a required dependency of Action Mailer (and Active Storage, I imagine, thought that seems to be missing the gem dep? cc @georgeclaghorn). It's required there, even if you don't want to do the work in a separate process, because AJ is the abstraction layer you use to express that preference (by using the inline adapter).

Commenting out the railtie is definitely supposed to work -- if you do that and then use actionmailer, it'll require it for you.

We should probably add some railties tests that do a basic generate-setup-and-boot with every combination of frameworks commented, in hope of catching this sort of thing... though as the main integration point with all the frameworks coming together, it's always going to be the hardest part to effectively cover.

@matthewd or @rafaelfranca - I am having trouble creating test that fails either on master or on the 5-2-stable branch. I'll continue to try and find differences between our production app that ran into this problem and the test dummy app template, but do you have any suggestions that stand out when looking at the temp commit linked above?

First thing I see is that you are testing in the wrong file. Engine.rb is for rails engines, you should be testing in the app. But other than that I'd expect that to fail. Maybe the application that is being booted is not the one were you removed the active job railtie?

@rafaelfranca - two things:

First, the bug was introduced in engine.rb which is why I am testing/fixing there. Gannon McGibbon only added ActiveJob processing of seeds within engines, so testing a change within the app doesn't make sense to my understanding. Could you explain a bit more what you have in mind so that I can understand it better?

Second, I was hoping that this change would be applied to any future Rails 5.2 bug fix releases. I made use of a modified version of the test file's restrict_frameworks which does not exist in the 5-2-stable branch, but that's probably not necessary (based on the logic of my bug fix, overriding the config alone should be sufficient). What is the best way to get this bug fix included both in future 5.2.x and 6.x releases? Are there special tags or branches I should be using with my commit/PR?

Hello,

This change has caused my seeds to execute jobs that do not execute under rails 5.2.2.

I use delayed job, and in my seeds I create some data, and then move long running tasks (like api requests) onto the queue.

This change has forced these jobs to begin executing immediately. I have one project that relies on this backgrounding to do several long tasks in parallel, and another that utilizes delayed job to clear specific jobs from the db (this issue has surfaced because that project uses webmock to make sure seeds don't hit external APIs).

On top of that, if in db/seeds.rb I inspect the current queue adapter via ApplicationJob.queue_adapter or Rails.application.config.active_job.queue_adapter, I am told that it is delayed_job. Forcing an exception in the job shows a trace that somehow the inline adapter taking over... Only when inspecting ApplicationJob.queue_adapter from within the job do I finally see inline is set. It has taken some time to find this execution path.

I am still trying to wrap my head around the root issue above. Perhaps if this is entirely a work around of not calling active job if it isn't there, perhaps the check should be simply to check if it is there, and to utilize whatever queue adapter is already set instead of forcing inline.

Hi folks, is there a timetable for the next patch release for Rails 5.2?

Apparently, this regression has been fixed. Without it, our seeding is completely broken, which is a nuisance. 😉